Add the Ability to Load and Save External Metadata - #173
Theo Geddes (mo-tgeddes) wants to merge 95 commits into
Conversation
… command line applications
Co-authored-by: Andrew Clark <2562650+arjclark@users.noreply.github.com> Signed-off-by: Theo Geddes <theo.geddes@metoffice.gov.uk>
Merge CallBackMetadata Class to feature branch
Updated load save feature branch
Cleanup and add fix for using constraints
Add check for misspelt license
Co-authored-by: Andrew Clark <2562650+arjclark@users.noreply.github.com> Signed-off-by: Theo Geddes <theo.geddes@metoffice.gov.uk>
Add copy metadata functionality
|
There is a metadata test that would usually be attached to the |
Josh Rackham (jrackham-mo)
left a comment
Josh Rackham (jrackham-mo)
left a comment
There was a problem hiding this comment.
Passing this back after a first review, there are still some areas I will need to look into in more detail (e.g. the saver side) but this should hopefully be enough on the loader side to be useful for you
| directly affect that data. For example, licensing information or attributions. | ||
|
|
||
| Some ancillary file formats cannot include metadata. Metadata can be very important to | ||
| keep alongside the data as often data will have rome form of requirement or restriction |
There was a problem hiding this comment.
| keep alongside the data as often data will have rome form of requirement or restriction | |
| keep alongside the data as often data will have some form of requirement or restriction |
| Metadata will only be saved to a sidecar file, if the ancil loader is used. This is | ||
| because NetCDF files will include the attributes within the file. | ||
|
|
||
| The sidecar files produced by ANTS follow the same naming covenstions at those loaded in |
There was a problem hiding this comment.
| The sidecar files produced by ANTS follow the same naming covenstions at those loaded in | |
| The sidecar files produced by ANTS follow the same naming coventions at those loaded in |
| Saving Metadata | ||
| --------------- | ||
|
|
||
| Metadata will only be saved to a sidecar file, if the ancil loader is used. This is |
There was a problem hiding this comment.
Just to check, should it be the ancil saver rather than loader that determines whether sidecar files are saved?
| Metadata will only be saved to a sidecar file, if the ancil loader is used. This is | |
| Metadata will only be saved to a sidecar file, if the ancil saver is used. This is |
There was a problem hiding this comment.
I really like this documentation, but I do wonder if it is really a tutorial as such, maybe it would make sense somewhere else? Perhaps under Ancillary Generation Pipeline? Alternatively, it could be made into a tutorial with some more examples
| Some ancillary file formats cannot include metadata. Metadata can be very important to | ||
| keep alongside the data as often data will have rome form of requirement or restriction | ||
| on it. Because of this, ANTS can handle in external metadata files, provided they match | ||
| the naming convention of `filename.attribute.accepted-metadata`. The current accepted |
There was a problem hiding this comment.
As discussed, can this be clarified a bit? It would be good to have a concrete example here, e.g. for file.pp, a license will be loaded from file.pp.license.
| the naming convention of `filename.attribute.accepted-metadata`. The current accepted | |
| the naming convention of `filename.extension.accepted-metadata`. The current accepted |
| """ | ||
| if type(filename) is list: | ||
| filename = filename[0] | ||
| metadata_filenames = "".join([filename, ".*"]) |
There was a problem hiding this comment.
Minor style recommendation, use an f-string for readability
| metadata_filenames = "".join([filename, ".*"]) | |
| metadata_filenames = f"{filename}.*" |
| return args, kwargs | ||
|
|
||
|
|
||
| class _CallbackMetadata(object): |
There was a problem hiding this comment.
| class _CallbackMetadata(object): | |
| class _CallbackMetadata: |
No need to inherit from object
|
|
||
|
|
||
| def ancil(cubes, filename): | ||
| def ancil(cubes, filename, ignore_writing_metadata_files=False): |
There was a problem hiding this comment.
Can I make a small suggestion for readability? It might be better to have write_metadata_files=True rather than ignore_writing_metadata_files=False since it avoids a double negative of if not ignore_writing_metadata_files meaning "write metdata files" (i.e. "do this" instead of "don't not to this")
| def ancil(cubes, filename, ignore_writing_metadata_files=False): | |
| def ancil(cubes, filename, write_metadata_files=True): |
I know what you've got here is consistent with the loaders, maybe it should be swapped over there as well?
| # Copy the metadata | ||
| copy_metadata_attributes(source, reference) | ||
| expected_msg = "a value not in the allowed list" | ||
| with pytest.raises(KeyError, match=expected_msg): |
There was a problem hiding this comment.
Could test this with the following?
assert "a value not in the allowed list" not in source.attributes| metadata_to_copy=[ | ||
| "license", | ||
| "attribution", | ||
| "restrictions", | ||
| "institution", | ||
| "acknowledgement", | ||
| "references", | ||
| ], |
There was a problem hiding this comment.
Beware of mutable defaults, maybe best to replace this with a tuple? Or replace with None and use a default list defined within the function (as in the example in the link)
3 participants
Closes #32, #33
To be completed prior to review request and updated as required during the review process.
If the answer to an item on the list is not applicable, feel free to replace the checkbox with 'N/A' to give extra clarity.
All developers are reminded to follow the ancil working practices
Branch
Related branches (e.g. ancillary-file-science):
[please link any related branches here]
ANTS rose stem logs
dev-ants-core/run136
ancillary-file-science rose stem logs
contrib/run34
Testing
For core ANTS only tests, the bare minimum that will be accepted is the
group=unittestsbut many, if not most, changes will need to test other groups to ensure they meet reviewer expectations. In general, it should be possible and is advised to run thegroup=allgroup prior to review submission as this will catch any consequential issues. Additionally you must run theancillary-file-sciencetests, pointing at your branch, withgroup=allto capture any behaviour changes affecting Science codes.If your change will alter existing science results, you will need to seek appropriate Scientific validation and confirm that the model has been initialised with your new development. Inspecting a change in xconv/pyplot/visualiser of choice is not sufficient to demonstrate the model can be initialised from your file.
Impact of change
cylc vip ./rose-stem -z group=alltestscylc vip ./rose-stem -z group=alltestsApprovals for this change
New functionality further testing
These tasks must succeed for your ticket to pass review.
Other
Rose stem logs
Please copy in the contents of your trac_status.log file(s) below (found in the cylc-run directory for your rose stem run) to your rose-stem testing here. Note: if your changes lead to a change in answers, you must run
cylc vip ./rose-stem -z group=allto help ensure all affected configurations has been flagged up.linkcheck currently failing - it looks unrelated
Test Results - Summary
Test Results - Detail
Test Results - Summary
Test Results - Detail
See Metoffice/ancillary-file-science#102 for explanation of failures