Skip to content
This repository has been archived by the owner on Jul 8, 2021. It is now read-only.

Add regridding functionality #10

Merged
merged 20 commits into from
Aug 17, 2020

Conversation

stephenworsley
Copy link
Contributor

Provides an API to ESMF regridding which is closer aligned to UGRID format, with a view to simplifying the process of regridding. Support is given to both lat-lon grids and UGRID style meshes, each of which can be used interchangably with respect to the regridder class.
Also, the regridder extracts weights from ESMF and implements its own regridding calculations. This is done so that ESMF objects are only needed while setting up the regridder and regridding can be done with scipy.sparse matrices, similar to current Iris regridding. This should make ESMF based regridding easier to align with other iris regridding schemes (this could, for example, allow easier weight caching, so that regridding can be done with precomputed weights without having to install ESMF).

@stickler-ci
Copy link

I couldn't find a .stickler.yml file in this repository. I can make one for you, or you can create one by following the documentation.

@pp-mo
Copy link
Member

pp-mo commented Aug 4, 2020

I guess this wants at least some testing before we merge.
We don't yet have Travis testing, we can maybe address that tomorrow/Thursday ..

@stickler-ci
Copy link

I couldn't find a .stickler.yml file in this repository. I can make one for you, or you can create one by following the documentation.

1 similar comment
@stickler-ci
Copy link

I couldn't find a .stickler.yml file in this repository. I can make one for you, or you can create one by following the documentation.

@trexfeathers
Copy link
Contributor

@stephenworsley are you able to add ESMF to the conda_requirements file, assuming it is available via conda? That would fix the Travis failure I think.

iris_ugrid/regrid.py Outdated Show resolved Hide resolved
iris_ugrid/regrid.py Outdated Show resolved Hide resolved
iris_ugrid/regrid.py Outdated Show resolved Hide resolved
iris_ugrid/regrid.py Show resolved Hide resolved
iris_ugrid/regrid.py Outdated Show resolved Hide resolved
iris_ugrid/tests/test_regrid.py Outdated Show resolved Hide resolved
iris_ugrid/tests/test_regrid.py Outdated Show resolved Hide resolved
iris_ugrid/tests/test_regrid.py Outdated Show resolved Hide resolved
iris_ugrid/tests/test_regrid.py Outdated Show resolved Hide resolved
iris_ugrid/tests/test_regrid.py Outdated Show resolved Hide resolved
@trexfeathers
Copy link
Contributor

@stephenworsley if you were to sort out black in this PR (similar to SciTools/iris#3518) then that should sort out those Stickler complaints

@stephenworsley
Copy link
Contributor Author

@trexfeathers Is it appropriate to handle black in this pull request or should that be handled in a seperate PR?

@trexfeathers
Copy link
Contributor

@trexfeathers Is it appropriate to handle black in this pull request or should that be handled in a seperate PR?

Agreed. See #14

iris_ugrid/tests/test_regrid.py Outdated Show resolved Hide resolved
iris_ugrid/tests/test_regrid.py Outdated Show resolved Hide resolved
Copy link
Member

@pp-mo pp-mo left a comment

Choose a reason for hiding this comment

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

Mostly comments about docstrings clarity (at least so far), and some rather finicky points so you should interpret quite broadly !

Basically, though, this is all much, much clearer now 🥇 💐 !

I guess the "regrid demo users" will be exposed to this, so it is worth trying to get the explanations fairly complete + clear.
But, don't let any of this delay us too much : good enough is good enough, and we can make further improvements later if needed.

Just as a final note on that : the docstring style you're using reads just fine, but it is not numpy conventions
( see : https://numpydoc.readthedocs.io/en/latest/format.html
as mentioned here in cube-arithmetic PR
I think we're planning to adopt numpy conventions throughout scitools in the future, even though Iris doesn't conform at the present )
I frankly wouldn't worry about that for now, it can absolutely be fixed later.

iris_ugrid/regrid.py Show resolved Hide resolved
iris_ugrid/regrid.py Outdated Show resolved Hide resolved
iris_ugrid/regrid.py Outdated Show resolved Hide resolved
iris_ugrid/regrid.py Outdated Show resolved Hide resolved
iris_ugrid/regrid.py Outdated Show resolved Hide resolved
iris_ugrid/regrid.py Show resolved Hide resolved
@pp-mo
Copy link
Member

pp-mo commented Aug 14, 2020

I think the structure of the tests is a bit haphazard.

But I don't know quite how to represent the
standard structures we use in Iris without the unittest TestCase classes.
In terms of the existing Iris tests organisation, there would be a 'test/units/regrid/' and within that a test_MeshInfo.py, test_GridInfo.py and test_Regridder.py. Then, your test_make_mesh would be test_MeshInfo.Test_make_esmf_field, while test_make_grid would be test_GridInfo.Test_make_esmf_field.
The regridder tests are rather more like the old style.

The only thing I think is practically missing is more specific + explicit testing of the GridInfo MeshInfo constructors.
But the testcases present do cover the essentials anyway.
It would be useful to add something here if you expand the usage of optional args, as you suggested you might do.

I would be generally interested to see how we could have a more obvious + regular arrangement using pytest tests, but I guess we don't have to do that now. @trexfeathers any ideas ??
I guess the answer would involve 'fixtures' in place of the tests-class + test-method structure of the old style.

@pp-mo
Copy link
Member

pp-mo commented Aug 14, 2020

Otherwise, I can find nothing wrong with the actual code, so I guess this is pretty near done 👍

@trexfeathers
Copy link
Contributor

I would be generally interested to see how we could have a more obvious + regular arrangement using pytest tests, but I guess we don't have to do that now. @trexfeathers any ideas ??

@pp-mo for me this is a perfect example of something that would be better to evolve organically as we get more tests that need organising/structuring - it would probably take extra effort to plan out before we have the anticipated spread of tests.

iris_ugrid/regrid.py Outdated Show resolved Hide resolved
@pp-mo pp-mo merged commit 5ec19d8 into SciTools-incubator:master Aug 17, 2020
@pp-mo pp-mo mentioned this pull request Aug 21, 2020
3 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants