Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
use cf-xarray heuristic to identify bound variables. Add test #54
use cf-xarray heuristic to identify bound variables. Add test #54
Changes from 2 commits
a585d9e
2bc0b21
4df99fb
c99e29f
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should add a description to the bounds variable too. Something like "bounds for the time coordinate" as in #51. We can construct an appropriate description string in the
if-elif-else
block before this and use that here.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So I wanted to hear your thoughts on this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good questions, here's what I think:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I'm happy adopting your perspective on 1 and 4. I'm less convinced about 2 and 3 and I believe we should spend some time thinking about the "metadata augmentation architecture" before writing too much code. I'm happy to add the description you mentioned, but I don't think this should serve as a template to add other kinds of information.
My discomfort comes from mixing process logic (NcML -> STAC) with thematic knowledge (metadata fixes) in the same code base. I think we can get the process logic fairly robust and maintenance should be minimal. The thematic knowledge will always grow with new datasets. Mixing both has the potential to generate maintenance work in the future.