Skip to content
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

Fix lat and lon bounds for FGOALS-g3 mrsos #1289

Merged
merged 10 commits into from
Aug 26, 2021
Merged

Fix lat and lon bounds for FGOALS-g3 mrsos #1289

merged 10 commits into from
Aug 26, 2021

Conversation

thomascrocker
Copy link
Contributor

@thomascrocker thomascrocker commented Aug 24, 2021

Description

Closes #1288

When loading the mrsos data, the fix checks monotonicity of the bounds on the latitude and longitude coords. If there is a problem it deletes the bounds and assigns new ones.


Before you get started

Checklist

It is the responsibility of the author to make sure the pull request is ready to review. The icons indicate whether the item will be subject to the πŸ›  Technical or πŸ§ͺ Scientific review.


To help with the number pull requests:

@codecov
Copy link

codecov bot commented Aug 24, 2021

Codecov Report

Merging #1289 (cadca28) into main (135fce2) will increase coverage by 0.05%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1289      +/-   ##
==========================================
+ Coverage   86.39%   86.44%   +0.05%     
==========================================
  Files         188      188              
  Lines        9167     9187      +20     
==========================================
+ Hits         7920     7942      +22     
+ Misses       1247     1245       -2     
Impacted Files Coverage Ξ”
esmvalcore/cmor/_fixes/cmip6/fgoals_g3.py 100.00% <100.00%> (ΓΈ)
esmvalcore/preprocessor/_io.py 85.10% <0.00%> (+0.85%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Ξ” = absolute <relative> (impact), ΓΈ = not affected, ? = missing data
Powered by Codecov. Last update 135fce2...cadca28. Read the comment docs.

Copy link
Contributor

@valeriupredoi valeriupredoi left a comment

Choose a reason for hiding this comment

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

cheers for the fix! Could you please have a look at my comment about the function and also, if you could please write a test for the fix, that'd be awesome. Let me know if you can't write the test, I'll do it! I have fixed the style so we're codacy-fine 😁

esmvalcore/cmor/_fixes/cmip6/fgoals_g3.py Outdated Show resolved Hide resolved
@thomascrocker
Copy link
Contributor Author

cheers for the fix! Could you please have a look at my comment about the function and also, if you could please write a test for the fix, that'd be awesome. Let me know if you can't write the test, I'll do it! I have fixed the style so we're codacy-fine grin

Happy to write a test, but could you point me to an example from another model so I know where to start!

@valeriupredoi
Copy link
Contributor

cheers for the fix! Could you please have a look at my comment about the function and also, if you could please write a test for the fix, that'd be awesome. Let me know if you can't write the test, I'll do it! I have fixed the style so we're codacy-fine grin

Happy to write a test, but could you point me to an example from another model so I know where to start!

awesome, cheers! Yeah, sorry, should have pointed you to the test file - have a look at tests/integration/cmor/_fixes/cmip6/test_fgoals_g3.py - all you'd have to do would be just to add a new test case there 🍺

@thomascrocker
Copy link
Contributor Author

OK, inspired very much by the existing code for testing other variables I have now added a test for the fix that I've added. It's passed the automatic checks at least. πŸ˜ƒ

Copy link
Contributor

@jvegreg jvegreg left a comment

Choose a reason for hiding this comment

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

There is just a small problem with the test, but the rest looks good

Comment on lines 141 to 142
assert _check_bounds_monotonicity(fixed_cube.coord('latitude'))
assert _check_bounds_monotonicity(fixed_cube.coord('longitude'))
Copy link
Contributor

Choose a reason for hiding this comment

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

You are checking the code by calling parts of the same code. That's bad practice, as some problems in the _check_bounds_monotonicity can go unnoticed

In fact, the check should be easier than that, something like

np.testing.assert_allclose(
        fixed_cube.coord('latitude').bounds, 
       [[ 0.5, 1.5], [1.5, 2.5], [2.5, 3.5]]
)

By the way, it would have been ok if there was a separate test for the _check_bounds_monotonicity function, but that would have been a complete overkill as we are just interested in checking if the data is correctly fixed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, that makes sense. I have modified the test to use your suggestion.

@jvegreg jvegreg added the fix for dataset Related to dataset-specific fix files label Aug 25, 2021
Copy link
Contributor

@valeriupredoi valeriupredoi left a comment

Choose a reason for hiding this comment

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

cheers @thomascrocker and @jvegasbsc - good work! Only thing left is to check relevant boxes in the PR description and remove or strikethrough the ones that dont apply 🍺

Copy link
Contributor

@jvegreg jvegreg left a comment

Choose a reason for hiding this comment

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

Ready to merge from my side, although I would remove the following two lines, as they are redundant:

assert _check_bounds_monotonicity(fixed_cube.coord('latitude'))
assert _check_bounds_monotonicity(fixed_cube.coord('longitude'))

@jvegreg jvegreg changed the title fixes for bounds problem for mrsos Fix lat and lon bounds for FGOALS-g3 mrsos Aug 25, 2021
@thomascrocker
Copy link
Contributor Author

Ready to merge from my side, although I would remove the following two lines, as they are redundant:

assert _check_bounds_monotonicity(fixed_cube.coord('latitude'))
assert _check_bounds_monotonicity(fixed_cube.coord('longitude'))

Happy to remove them, as I agree they are not really needed, but codecov complained that the return true line in the function was not covered by the tests. If it's OK to merge with it failing the codecov check then I can remove them.

@jvegreg
Copy link
Contributor

jvegreg commented Aug 25, 2021

Happy to remove them, as I agree they are not really needed, but codecov complained that the return true line in the function was not covered by the tests. If it's OK to merge with it failing the codecov check then I can remove them.

Better in this case is to create a copy of the test with monotonic bounds and check that they are still okay after the test. Just in case the data is ammended

@thomascrocker
Copy link
Contributor Author

thomascrocker commented Aug 25, 2021

Happy to remove them, as I agree they are not really needed, but codecov complained that the return true line in the function was not covered by the tests. If it's OK to merge with it failing the codecov check then I can remove them.

Better in this case is to create a copy of the test with monotonic bounds and check that they are still okay after the test. Just in case the data is ammended

OK, I have added another test that supplies a dummy cube with coords that don't need fixing.

I'm not authorised to merge so @valeriupredoi or @jvegasbsc will need to do it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fix for dataset Related to dataset-specific fix files
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Dataset problem: Coordinate issue with FGOALS-g3 mrsos
3 participants