Skip to content
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

Metadata to encode lossy compression (by quantization) with @cofinoa #519

Open
wants to merge 36 commits into
base: main
Choose a base branch
from

Conversation

czender
Copy link
Contributor

@czender czender commented Apr 11, 2024

See issue #403 for discussion of these changes.

Release checklist

  • Authors updated in cf-conventions.adoc? Add in two places: on line 3 and under .Additional Authors in About the authors.
  • Next version in cf-conventions.adoc up to date? Versioning inspired by SemVer.
  • history.adoc up to date?
  • Conformance document up to date?

For maintainers

After the merge remember to delete the source branch.
Tags are set at the conclusion of the annual meeting; until then, main always is a draft for the next version.

@czender czender marked this pull request as ready for review April 17, 2024 19:33
@czender
Copy link
Contributor Author

czender commented Apr 17, 2024

Hi @davidhassell and @cofinoa . Please review this PR. I decided to omit the sections on optional keywords for pre- and post-metrics like relative precision. I would prefer to start small with the core of the proposal which is self-contained here. Other options can of course be added at your discretion now or later.

@czender czender linked an issue Apr 18, 2024 that may be closed by this pull request
@czender czender marked this pull request as draft June 27, 2024 00:20
@czender czender marked this pull request as ready for review June 27, 2024 01:25
@czender
Copy link
Contributor Author

czender commented Jun 27, 2024

@davidhassell and @JonathanGregory I have addressed and merged the comments from Jonathan's recent review. Please take another look and LMK what further changes are desired. (FYI I have also updated the reference implementation in NCO to adhere to this updated version).

Copy link
Contributor

@davidhassell davidhassell left a comment

Choose a reason for hiding this comment

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

Hi Charlie,

I really enjoyed reading this in detail. Hopefully all my comments are self explanatory, or are sufficiently explained.

They're either asciidoc formating; personal style suggestions (feel free to treat as you will); or - of more consequence - some suggestions in should/must, and clarifying (?) what's allowed. I'll be interested to hear what you think of the last category!

Cheers,
David

ch01.adoc Outdated Show resolved Hide resolved
history.adoc Outdated Show resolved Hide resolved
ch08.adoc Outdated Show resolved Hide resolved
ch08.adoc Outdated Show resolved Hide resolved
ch08.adoc Outdated Show resolved Hide resolved
ch08.adoc Outdated Show resolved Hide resolved
ch08.adoc Outdated Show resolved Hide resolved
ch08.adoc Outdated Show resolved Hide resolved
conformance.adoc Outdated Show resolved Hide resolved
ch08.adoc Outdated Show resolved Hide resolved
@czender
Copy link
Contributor Author

czender commented Jul 26, 2024

@davidhassell Thank you for your careful review. You found many ways to make the new sections more clear and precise. I appreciate the time you spent. The review came with some outdated suggestions from April, which I finally just resolved without comment. Overall, I agree with ~90% of your suggested changes. However, as I mention in my first comment, I need guidance on whether to "Commit Suggestion" on those or merge them into my branch. Does "Commit Suggestion" actually commit the changes to my branch? I am worried that if I "Commit Suggestion" then the PR will have the changes and my branch will not and then I will be unable to create a new Convention document until the whole PR is actually merged. OK, those are just GitHub newbie issues.
The two areas that I need to hear back from you on are the ancillary_variables question I pose, and what to do with the Recommendation for the structure of the implementation attribute. Once we agree with what to do with those two issues, then all your suggestions will have been addressed.
Charlie

@davidhassell
Copy link
Contributor

Hi Charlie,

Just to say that "commit changes" does indeed merge the change into your branch. I'll pick this up on Monday, if not before. Have a good weekend!

czender and others added 5 commits July 26, 2024 12:35
Co-authored-by: David Hassell <[email protected]>
Sylistic or formatting suggested by DH

Co-authored-by: David Hassell <[email protected]>
Co-authored-by: David Hassell <[email protected]>
Co-authored-by: David Hassell <[email protected]>
Co-authored-by: David Hassell <[email protected]>
czender and others added 15 commits July 26, 2024 13:05
Co-authored-by: David Hassell <[email protected]>
Co-authored-by: David Hassell <[email protected]>
Co-authored-by: David Hassell <[email protected]>
Co-authored-by: David Hassell <[email protected]>
Co-authored-by: David Hassell <[email protected]>
Co-authored-by: David Hassell <[email protected]>
Co-authored-by: David Hassell <[email protected]>
Co-authored-by: David Hassell <[email protected]>
Co-authored-by: David Hassell <[email protected]>
Co-authored-by: David Hassell <[email protected]>
Co-authored-by: David Hassell <[email protected]>
Co-authored-by: David Hassell <[email protected]>
Co-authored-by: David Hassell <[email protected]>
Co-authored-by: David Hassell <[email protected]>
Co-authored-by: David Hassell <[email protected]>
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.

Metadata to encode quantization properties
2 participants