-
Notifications
You must be signed in to change notification settings - Fork 100
Samos misc ancillaries #2133
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
Samos misc ancillaries #2133
Conversation
* Avoid cubelists of cubelists. * Replace np.product with np.prod. * Replace np.int. * Replace np.NAN with np.nan. * Replace np.NAN with np.nan. * Replace np.NaN with np.nan. * Replace np.product with np.prod. * Replace assertRaisesRegexp with assertRaisesRegex and use collections.abc.Callable instead of collections.Callable. * Simplify test in test_flatten.py to avoid cubelist within a cubelist. * Replace Cube(None) with an alternative. * Unpin environments for testing. * Remove pygam version. * Update improver_a.yml * Remove non-essential dependencies from yml files and add pins. * Minor edits following review comments. * Modify docstring to better reflect the inputs provided.
* Updates to cube_combiner for new environment * Update checksums
| def generate_roughness_length_at_sites( | ||
| roughness_length: Cube, neighbour_cube: Cube | ||
| ) -> Cube: | ||
| """Generate a roughness length ancillary cube at the site locations. This performs a | ||
| spot extraction of the roughness length data at the site locations. | ||
| Args: | ||
| roughness_length: | ||
| A cube containing the roughness length data. | ||
| neighbour_cube: | ||
| A cube containing information about the spot data sites and | ||
| their grid point neighbours. | ||
| Returns: | ||
| A cube containing the roughness length at the site locations. | ||
| """ | ||
| return SpotExtraction(neighbour_selection_method="nearest")( | ||
| neighbour_cube, roughness_length | ||
| ) |
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've not reviewed this properly yet, but I have noticed that the produced vegetative_roughness_length cube contains time, forecast_reference_time and forecast_period coordinates:
vegetative_roughness_length / (m) (spot_index: 14883)
Dimension coordinates:
spot_index x
Auxiliary coordinates:
altitude x
latitude x
longitude x
met_office_site_id x
wmo_id x
Scalar coordinates:
forecast_period 0 seconds
forecast_reference_time 2017-06-13 06:00:00
time 2017-06-13 06:00:00
Attributes:
Conventions 'CF-1.7'
STASH m01s03i026
model_grid_hash '9766b1eb83e154c2cf1687d7d05f5583a227f68b89e44b74fefc7028c6d995cb'
source 'Data from Met Office Unified Model'
title 'unknown'
um_version '10.4'
This means that the EMOS and SAMOS CLIs fail to identify this cube as an additional predictor. Can time-related coordinates be removed please?
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.
maxwhitemet
left a comment
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.
Thanks @mspelman07.
I think the PR is ready to move to second review after these very minor adjustments.
improver/generate_ancillaries/generate_miscellaneous_ancillaries.py
Outdated
Show resolved
Hide resolved
improver/generate_ancillaries/generate_miscellaneous_ancillaries.py
Outdated
Show resolved
Hide resolved
improver/generate_ancillaries/generate_miscellaneous_ancillaries.py
Outdated
Show resolved
Hide resolved
improver/generate_ancillaries/generate_miscellaneous_ancillaries.py
Outdated
Show resolved
Hide resolved
improver/generate_ancillaries/generate_miscellaneous_ancillaries.py
Outdated
Show resolved
Hide resolved
improver_tests/generate_ancillaries/test_miscellaneous_ancillaries.py
Outdated
Show resolved
Hide resolved
improver_tests/generate_ancillaries/test_miscellaneous_ancillaries.py
Outdated
Show resolved
Hide resolved
improver/generate_ancillaries/generate_miscellaneous_ancillaries.py
Outdated
Show resolved
Hide resolved
improver/generate_ancillaries/generate_miscellaneous_ancillaries.py
Outdated
Show resolved
Hide resolved
|
Thank you @mspelman07. I am happy with the changes. I'm moving this ticket to second review for @brhooper to have under their profile so they can merge the associated PRs when the Samos feature branch is ready, as you have indicated the need for. |
|
@mspelman07 and @brhooper I provide a link below to the Makefile in the ancils repository. https://github.com/MetOffice/improver_ancil/blob/master/Makefile The make file can currently be run to reconstruct all of our derived ancillaries, meaning if we change a site list, or get an updated orography, etc. all the ancillaries derived from these basic ancils can be reproduced in their updated form. We need to ensure that any new ancillaries being produced for SAMOS are added to this Makefile so we don't find ourselves unable to reproduce them or update them without wading through notebooks. We could consider doing this after the release, but nice to have it for when we start panicking and making changes close to the freeze date. |
bayliffe
left a comment
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.
The various ancil generation codes need CLI interfaces, ideally with our preferred settings as default values. These can then be integrated into our ancil Makefile allowing these to be remade trivially when we change the sitelists or other basic inputs.
If there are inputs that are essentially static (shapefiles etc.) that need to be kept, these should be added to the improver_ancil repo unless their size is prohibitive (more than a few MB), in which case we should find an alternative location.
brhooper
left a comment
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.
Thanks @mspelman07, I've added one comment for you to look at.
| """Test a ValueError is raised if the chosen smoothing coefficient limits | ||
| are outside the range 0 to 0.5 inclusive.""" |
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 don't think that this doc-string is describing this test.
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.
True, I've corrected this
…d to work. Still requires additional unit tests for new functionality.
* samos_ancil_distance_to: Format fix. Add a test for using a projection that is unsuitable for the sites being processed. Remove hardcoded projection for distance to calculation. Tests updated to work. Still requires additional unit tests for new functionality. Update dictionary call
… an EPSG projection code.
…nd is needed here specifically for the corinne surface type ancillary that has no spatial coordinate bounds. Adds a simple test to demonstrate that this works.
…ue is used and when not.
…lution of the input data, rather than coarsening this first, which yields more accurate land fraction estimates.
…seful as the neighbour cube has missing altitudes filled in from an orography, ensuring all of the definitions are complete.
…tion so we can be sure that the metadata of the resulting cube, specifically the altitude coordinate, is complete.
brhooper
left a comment
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.
Thanks @bayliffe, this all looks good to me.
maxwhitemet
left a comment
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.
Happy with the changes made. Approved 👍











Addresses: https://github.com/metoppv/mo-blue-team/issues/902
This PR builds onto the DistanceTo PR but adding in the generation of a selection of additional ancillary files. These both should be merged into the Samos feature branch when it is ready
Testing: