Skip to content
This repository has been archived by the owner on Apr 24, 2020. It is now read-only.

Add support for groups #8

Merged
merged 4 commits into from
Nov 21, 2019
Merged

Conversation

erget
Copy link
Member

@erget erget commented Jun 26, 2019

This update is to bring the Conformance document in line with the discussion in cf-convention/cf-conventions#144, as implemented in cf-convention/cf-conventions#145.

@erget
Copy link
Member Author

erget commented Jul 17, 2019

@JonathanGregory would you be willing to moderate this pull request? It would seem to make sense to me as you're most familiar with the Groups proposal.

@JonathanGregory
Copy link

Ros has studied the implication for conformance checking and will post some comments on the draft requirements.

@RosalynHatcher
Copy link
Contributor

@erget I've looked at the proposal and have a couple of general comments. The conformance document is intended to be a list of requirements/recommendations that an application that checks files for CF conformance to use it so shouldn't contain items that are subjective like "Groups should be named in such a way that human readers can interpret them" - this is impossible to check. Items should be written such that they are self contained checks. For example, an application is not going to know when a path should be absolute or relative, but it can check that the path conforms to a specific format.

Overall I think the proposal contains all the required points but just needs some rewording. I shall attempt to put my suggestions inline rather than recreate in this comment.

Additionally, this also impacts other areas of the conformance document, where variable/dimension references can now be paths not just single words. Does the groups proposal expect that these all may contain out-of-group references? E.g. cell boundaries, cell measures, etc I think this probably needs to be clarified in the conventions document so will add a comment on cf-convention/cf-conventions#144

@RosalynHatcher
Copy link
Contributor

RosalynHatcher commented Jul 25, 2019

Ok - Inline was turning into a complete mess! My suggestions are:

Remove - they are subjective and cannot be checked:

  • Group names must not embed any metadata that is not redundantly stored in any of the group attributes.
  • Groups should be named in such a way that human readers can interpret them.

Remove - already covered elsewhere:
Covered by checks on Appendix A, by recommendation in 2.6.2 and by requirement 4 in proposal.

  • Group attributes must not be used to store metadata normally encoded as per-variable attributes, such us _FillValue, scale_factor, add_offset, valid_min, valid_max, and valid_range.
  • The title and history attributes may be defined in non-root groups to provide additional provenance and description of the subsidiary data.
  • Auxiliary coordinate variables must reside in the referring group or one of its direct ancestors.

2.6.2:
Think this should include refrence to global attributes and would be simpler written as:

  • The title and history attributes are only defined as global or group attributes. If they are used as per variable attributes……

Suggestion for the remainder mainly consolidating the path syntax into one item:

2.7 Groups:
Requirements:

  • The Conventions and external_variables attributes must not be used in non-root groups
  • If any dimension of an out-of-group variable has the same name as a dimension of the referring variable, the two must be the same dimension (ie. they must have the same netCDF dimension ID).
  • Variable or dimension paths must follow a UNIX style file convention. They must be formed of words (composed of letters, digits and underscores) and be separated by the slash character (‘/’). Paths may begin with either ‘/’, ‘...’ or a word.
  • The variable or dimension referenced must exist in the file unless it is an external variable. References can be absolute, relative or with no path, in which case, the variable or dimension must be found in one of the following (in order of precedence):
    • In the referring group
    • In the ancestor group (starting from the direct ancestor and proceeding toward the root group, until it is found)
    • By the lateral search algorithm for coordinate variables only.

Recommendations:

  • NUG coordinate variables that are not in the referring group or one of its direct ancestors should be referenced by absolute or relative paths rather than relying on the lateral search algorithm.

Copy link
Member Author

@erget erget left a comment

Choose a reason for hiding this comment

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

Thanks - this is clearer than before.

@erget
Copy link
Member Author

erget commented Nov 20, 2019

Hi @JonathanGregory @RosalynHatcher as far as I understand this is ready to merge, is that correct? @dblodgett-usgs had merged in the changse to the Conventions themselves, thus closing the associated issue, I believe under the assumption that this pull request was already finished, but it seems nobody ever pushed the red button (it probably got overlooked because this is in a different repository). If there are no objections I propose merging these changes, as we've more than 6 weeks of silence.

@JonathanGregory
Copy link

Yes, I agree, that's fine. Thanks to Ros and Daniel.

@erget
Copy link
Member Author

erget commented Nov 21, 2019

@dblodgett-usgs would you mind merging this? I don't have write permission to the repo.

@dblodgett-usgs dblodgett-usgs merged commit 88cb54c into cf-convention:master Nov 21, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants