Skip to content

Add consensus documentation #9239

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 6 commits into from
Mar 25, 2025
Merged

Add consensus documentation #9239

merged 6 commits into from
Mar 25, 2025

Conversation

zhiltsov-max
Copy link
Contributor

Motivation and context

Related #8434
Closes #8401

  • Added documentation for the consensus feature

How has this been tested?

Checklist

  • I submit my changes into the develop branch
  • I have created a changelog fragment
  • I have updated the documentation accordingly
  • I have added tests to cover my changes
  • I have linked related issues (see GitHub docs)

License

  • I submit my code changes under the same MIT License that covers the project.
    Feel free to contact the maintainers if that's a concern.

@zhiltsov-max zhiltsov-max requested a review from bsekachev as a code owner March 20, 2025 17:55
@zhiltsov-max zhiltsov-max requested a review from SpecLad March 21, 2025 08:13

## Basics

If you want to improve quality of your annotations, there are several widespread ways
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
If you want to improve quality of your annotations, there are several widespread ways
If you want to improve the quality of your annotations, there are several widespread ways

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you missed this?

Copy link
Contributor Author

@zhiltsov-max zhiltsov-max Mar 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not convinced this is a correct case for "the", nor I could find a reliable source where using "the" in this case is justified. From my perspective, "quality" is an uncountable noun in this context. It can be countable, but not in this meaning.

https://en.wiktionary.org/wiki/quality - meaning breakdown with countability

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is uncountable, but that only matters for indefinite articles. In this case, I believe using "the" is justified, as you're referring to a specific kind of quality.

In addition, Google Ngram shows that the variant with "the" is significantly more common.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, let's trust the majority voting then.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can see compression artifacts. Please retake these screenshots as PNG.

Copy link
Contributor Author

@zhiltsov-max zhiltsov-max Mar 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated, it was just 30% quality setting

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But they're still JPEGs. Why not save them as PNG?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because the original pngs are 2x of the current size

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have you tried processing them with zopflipng? That can yield significant size reduction without any quality loss.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It could achieve the size of 70% jpgs, winning us about 10kb. Actually, I think it makes sense to run this tool for all pngs in the repository.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, I think it makes sense to run this tool for all pngs in the repository.

Eh, I'd rather not add new binary blobs to the repository if the contents are equivalent. (I'd be up for replacing JPEG screenshots with PNG, though.)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By the way, maybe we should use WebP going forward. I just tried to recompress these images out of curiosity, and cwebp -z 9 brings the total size down by 60%, with zero quality loss. 🤯

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I experimented a little bit with webp encoding when I tried to replace pngs for HP. I discovered noticeable compression artifacts, for instance around curve objects. Maybe it was because of specific settings.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, WebP has both lossy and lossless variants, so you have to be careful. The -z option enables lossless encoding.

Copy link

@SpecLad SpecLad merged commit 4b72673 into develop Mar 25, 2025
15 checks passed
@SpecLad SpecLad deleted the zm/consensus-docs branch March 25, 2025 16:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants