-
Notifications
You must be signed in to change notification settings - Fork 300
Prioritise dim coord in _get_lon_lat_coords()
#5029
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
Prioritise dim coord in _get_lon_lat_coords()
#5029
Conversation
for more information, see https://pre-commit.ci
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.
This looks good.
My comments here are mostly about the style of the tests. These seem to be more in line with pytest, which we have relatively recently adopted, so I'm not against a stylistic change in principle. If there is an example in our code of tests in this style I would be interested in comparing it. Though if this is setting a precedent then I think it might want a broader discussion about what precedents we want to set before departing from our existing style.
The existing style, as far as I'm aware, involves nesting test functions in classes and having all code executed either within the test function or within setup and teardown methods.
lib/iris/tests/unit/analysis/cartography/test__get_lon_lat_coords.py
Outdated
Show resolved
Hide resolved
lib/iris/tests/unit/analysis/cartography/test__get_lon_lat_coords.py
Outdated
Show resolved
Hide resolved
|
@PaytonLeedham is going to give this a once over since we're all still getting used to pytest! |
for more information, see https://pre-commit.ci
…n for better PyTest consistency.
for more information, see https://pre-commit.ci
stephenworsley
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.
Looks good, just one minor point on the use of yield vs return. As far as I'm aware, using yield causes pytest to use a little extra machinery to remember and run the rest of the code in these fixtures. I'm not sure of all the details of how pytest deals with yield with no code afterwards vs how it deals with return, but it seems like using return is the simpler case and therefore better practice to use when yield is not necessary. This seems to be the pattern in the pytest documentation.
lib/iris/tests/unit/analysis/cartography/test__get_lon_lat_coords.py
Outdated
Show resolved
Hide resolved
lib/iris/tests/unit/analysis/cartography/test__get_lon_lat_coords.py
Outdated
Show resolved
Hide resolved
lib/iris/tests/unit/analysis/cartography/test__get_lon_lat_coords.py
Outdated
Show resolved
Hide resolved
lib/iris/tests/unit/analysis/cartography/test__get_lon_lat_coords.py
Outdated
Show resolved
Hide resolved
lib/iris/tests/unit/analysis/cartography/test__get_lon_lat_coords.py
Outdated
Show resolved
Hide resolved
lib/iris/tests/unit/analysis/cartography/test__get_lon_lat_coords.py
Outdated
Show resolved
Hide resolved
for more information, see https://pre-commit.ci
|
One more thought, it might be worth adding a note here to say which type of coordinate you are prioritising: iris/lib/iris/analysis/cartography.py Line 581 in d56824c
Something like: |
stephenworsley
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.
Looks good!
🚀 Pull Request
Closes #3916.
Added dim coord prioritisation to
_get_lon_lat_coords()iniris.analysis.cartography. This allowsiris.analysis.cartography.area_weights()andiris.analysis.cartography.project()to handle cubes which contain both dim and aux coords of the same type e.g.longitudeandgrid_longitude.