Skip to content

Fix function#2389

Open
maxwhitemet wants to merge 2 commits into
metoppv:masterfrom
maxwhitemet:tidy_ancil_generation
Open

Fix function#2389
maxwhitemet wants to merge 2 commits into
metoppv:masterfrom
maxwhitemet:tidy_ancil_generation

Conversation

@maxwhitemet

Copy link
Copy Markdown
Contributor

Addresses https://github.com/metoppv/mo-blue-team/issues/1204

Fixes an accidental line break in a docstring and ensures the resulting cube has an appropriate name.

Testing:

  • Ran tests and they passed OK

@mo-jbeaver mo-jbeaver left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I added a few small comments but overall happy with the changes.
The unit tests all ran as expected. However, the unit test should be updated to include the new variable added to the generate_roughness_length_at_sites function, checking that the pytest still runs as expected when ignore_grid_match = False and True.
Additionally, there probably should be an acceptance test that goes with these functions.

related coordinates.

Args:
roughness_length:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
roughness_length_cube:

neighbour_cube:
A cube containing information about the spot data sites and
their grid point neighbours.
ignore_grid_match:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If adding this new variable, should the unit and acceptance tests also be updated too, to check they still run as expected.

land_fraction = land_fraction.concatenate_cube()
land_fraction.rename("land_area_fraction")

return land_fraction

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
return land_fraction
return land_area_fraction

I don't quite understand the additional line land_fraction.rename("land_area_fraction"), maybe the last line should be this?

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.

2 participants