Add the Ability to Load and Save External Metadata - #173
Theo Geddes (mo-tgeddes) wants to merge 100 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
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
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
There was a problem hiding this comment.
I think adding the information to the loading and saving tutorial in #179 is probably the best place for it to go
|
|
||
|
|
||
| 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?
There was a problem hiding this comment.
The reason it is false, is that for the command line scripts, you can pass in --ignore-metadata-files to turn it off so it evaluates to true. In my head flags like that should evaluate to true, but if thats only me then I'm happy to change it :).
(And both need to evaluate to the same value for the command line scripts to correctly load and save.)
| # 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)
There was a problem hiding this comment.
Good catch thanks :). I've updated it to be defined in the function f983dfb.
Co-authored-by: Josh Rackham <144251043+jrackham-mo@users.noreply.github.com> Signed-off-by: Theo Geddes <theo.geddes@metoffice.gov.uk>
This branch has not been deployed
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