Bilinear/nearest-neighbour regridding with land-sea mask #1438
Conversation
fix a indent mistake
A number of FIXME comments have been left - these are items to address before submitting an upstream pull request
zfan001
added
MO review required
Codecov Report
@@ Coverage Diff @@
## master #1438 +/- ##
==========================================
+ Coverage 97.77% 97.84% +0.06%
==========================================
Files 101 107 +6
Lines 8916 9362 +446
==========================================
+ Hits 8718 9160 +442
- Misses 198 202 +4
Continue to review full report at Codecov.
|
zfan001
changed the title
|
Hi Stephen, |
|
I re-designed the unit test case to ensure code coverage. I also add a test for coordinate system conversion. Kind regards, Julian |
There was a problem hiding this comment.
Thanks @zfan001 for this work. This is a large PR and I don't understand all of the technical details. My review is focussed on software and code style.
A few general comments:
- There seems to be a missing
__init__.pyfile in regrid. - The docstrings could be better formatted. For example, sentences are usually capitalised with periods at the end. There are quite a few double spaces. (These can be found, e.g., in Vim with this regex
[^\s] [^\s#]. The hash is there to ignore inline comments formatted by Black.) See these resources too: PEP 257 and Google Python style guide. - I may have made some suggestions/comments on existing code that was moved in the PR. Feel free to ignore those if it's not relevant to this PR.
- The unit tests are mostly done via the cli interface. E.g. there are no unit tests of the functions in bilinear.py, idw.py, landsea2.py, or nearest.py. It might be good to add a few unit tests for these functions directly.
There was a problem hiding this comment.
Hey Julian,
Tried to cover off as much detail as possible but sure I've missed something given the size of the PR, but it all looks generally good! I've mainly focused on the Bilinear regridder with only a quick check of the nearest regridder, but the review was growing in size... thought it was worth putting out these comments first before checking Nearest in more detail. I know @fionaRust is also taking a look.
Summary:
- Bilinear regridder checks out and is as described 👍
- Understanding all assumptions about the input data is the most important thing missing from my point of view, maybe add in some optional validation of the data? (Think there is a hidden assumption about all coordinates being ascending, a stated assumption regarding the equal spacing of the source and no extrapolation(?)).
- A bit of refinement of the structure of the code added would help massively making it approachable for some looking at this in the future and maintaining it.
- A few area's where I wasn't clear about the intended behaviour, particularly in the edge cases of surface miss matches, I've just asked questions around, as I don't know if it's really right or wrong.
|
Hi @zfan001 I've started review this today and I'll continue tomorrow but I have a couple of questions in the meantime:
Thanks |
|
Hi Daniel, Aled & Fiona, @dmentipl @owena11 @fionaRust Thank you very much for your review. I came back to work this Monday after a long break. I am working on some code changes based on your feedback, and will reply to each item soon. @Fiona, Jupyter Notebooks and acceptance test data will be ready tomorrow. I will submit them for merging soon. Tom will help to make the files accessible to you. Kind regards, |
|
@zfan001 , No hurry from my side (I'm on leave next week 24th - 4th), but you've answered lots of my comments from the first pass of reviewing it. Any code changes to push up from them? So I can 'resolve conversations' and take another look at this. |
|
Code modifications have been made according to your suggestions (commit 3986b5a ). BTW, I created a pull request for adding four jupyter notebooks in improver_aux. Kind regards, Julian |
|
Created #1457 to track the follow on changes we're discussing on this PR so they don't get lost in the |
|
There is a tiny spacing difference due to rounding error in Australia domain grid. Changes have been made for considering it. Kind regards, Julian |
|
@fionaRust Thanks for the new issues you raised. I fixed most of them, and will do more changes soon. WGS84-related change will be considered in the follow-up PR. it seems that set_up_variable_cube only generates square-grid-box cubes. In the unit test, we designed rectangle-grid-box, and therefore did not use this function. Anyway, it should not affect the unit test. |
|
It is nice to see that both codecov/patch and codecov/project are improved. Only 4 lines are not run in the unit test. It is mainly due to the carefully designed unit test cases in which every scenario is turning up and every path is covered. |
There was a problem hiding this comment.
Final few comments to address from @fionaRust , but PR is looking in much better shape now, thanks for addressing the review comments Julian! It's dragged on longer than is pleasant for anyone, nice to be close to the end now 👍 No more comments from me, all covered by the follow on tickets etc.
Taken the AT data provided by Tom, unittest and acceptance tests seem to all run fine on my MO machine.
There was a problem hiding this comment.
Thanks Julian. This is really close now, thank you very much for all your work on this and your effort to address all your comments. This PR is adding a lot of good functionality to IMPROVER.
Only a few comments left from my latest review. The only one that is essential to address is using the set up cube function in the unit tests.
|
I fixed three issues today, and did not add the extra acceptance test. I would like to leave it to the follow-up PR to avoid the acceptance data transfer at this stage. All cube creation in the unit test uses "set_up_variable_cube" plus some work-around for rectangle grid. I did many tests, and it works well. The small PR has been merged. Two changed files in that PR are integrated into this PR as well, and so there should be no problem when merging this PR. Please check what I did today. Hope that everything goes fine, and this PR can be merged soon. Thanks |
* upstream/master: Move wiki to developer guide in docs (metoppv#1456) IMPRO-2026: Additional site identifier for spot-cubes and tweaks to existing metadata (metoppv#1458) Bilinear/nearest-neighbour regridding with land-sea mask (metoppv#1438) Add tolerance in grid even-spacing checking & calculation (metoppv#1459) Removes unused import statements (metoppv#1461) Fix metadata interpreter acceptance tests skipping (metoppv#1455) # Conflicts: # improver/standardise.py
* new regridder * add contributor * Update standardise.py fix a indent mistake * Cleanup, refactor and comment new regridding code A number of FIXME comments have been left - these are items to address before submitting an upstream pull request * fix new bugs * fix refactor bugs, add unit test * fix sorting using isort * fix codevec low-hit * add acceptance test * modifications based on reviewers comments * update SHA256SUMS * update SHA256SUMS * consider identical source and target domain * update SHA256SUMS for acceptance test * update SHA256SUMS * add unit test for better codecov * update to improver_py37_iris24 env * existing functions replaces new functions * edit docstring * minor change of docstring * improve calculate_grid_spacing function * minor fixing plus grid spacing test * add test_grid.py * add more unit test plus minor fixings * refactor grid.py and unit test code * update gap_spacing change Co-authored-by: Tom Gale <tom.gale@bom.gov.au>
Description
New Code for Regridding with Land-Sea Mask [Author: Zhiliang(Julian) Fan & Tom Gale]
(1) Add new finite-element-based bilinear regridding with land-sea mask awareness. This method is more suitable for smooth continuous variables than the nearest neighbour regridding method. The implementation is very fast (only 10-15% increase on computational time as compared with the bilinear regridding without considering land-sea mask).
(2) Add new implementation of nearest neighbour regridding with land-sea mask awareness. This new implementation achieved a significant speed-up as compared with the original implementation.
(3) Add new implementation of nearest-neighbour regridding, which is much faster than the original Iris version.
(4) Add new implementation of bilinear regridding, which achieved same results and similar computational time as the Iris version.
(5) A new directory ./improver/regrid is created for regridding. Old regridding code (in ./improver/standardise.py)and new regridding code are moved into this directory, and re-organized into several files. A unit testing directory ./improver_tests/regrid also created accordingly.
(6) The implementation is extended from the existing RegridLanSea options("bilinear","nearest","nearest-with-mask"). New options are "nearest-2","bilinear-2","nearest-with-mask-2","bilinear-with-mask-2" (see ../regrid/landsea.py)
(7) New unit test is added by a careful-designed regridding example, in which so many different scenarios and combinations turn up (passed).
(8) New acceptance test cases are also added for four new options respectively.
(9) The code assumes that the source grid is in rectilinear latitude/longitude coordinate system, and the target grid could be in rectilinear latitude/longitude coordinate system or Lambert Azimuthal Equal Area coordinate system ( a curvilinear coordinate system).
(10) Five Jupyter notebooks are available for this new feature, which will be added into improver-aux.
Testing:
CLA