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

Address review comments #36

Merged
merged 5 commits into from
Oct 10, 2022
Merged

Address review comments #36

merged 5 commits into from
Oct 10, 2022

Conversation

gadomski
Copy link
Contributor

@gadomski gadomski commented Oct 7, 2022

Related Issue(s):

Description:

Is it worth adding something like source to the roles of the NetCDF assets that are on the collections?

Done.

Use of an average datetime (e.g., middle of a year) instead of setting it to null: is that the recommended practice? I've been avoiding setting datetime for long time period data since the data is not necessarily representative of state as of a single date.

I was including the datetime because that's the value of the singular time value in the data array. However, I think agree with you that the start/end setup is better for expressing what these data are. Fixed.

Any opinions on use of singular rather than plural for units? For example, degree Celsius versus degrees Celsius.

I pulled these values straight from the NetCDFs so (IMO) it's not worth the effort to remap. If there were a "best practices for units" document that we could align to then it'd be worth it, but since we don't have that

spatial resolution in raster:bands?

We're in degree-based grids for everything except sea ice concentration, so the forced meter units for spatial_resolution is kind of non-sensical. Added for sea ice.

sci:citation on the collections: fill in or delete: [indicate subset used] and Accessed [date]

Done.

PR checklist:

  • Code is formatted (run scripts/format).
  • Code lints properly (run scripts/lint).
  • Tests pass (run scripts/test).
  • Documentation has been updated to reflect changes, if applicable.
  • Examples have been updated to reflect changes, if applicable

@gadomski gadomski requested a review from pjhartzell October 7, 2022 21:23
@gadomski gadomski merged commit 4c0a328 into main Oct 10, 2022
@gadomski gadomski deleted the issues/35-review-comments branch October 10, 2022 15:30
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.

Review Comments
2 participants