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

Lazy regridding with Linear, Nearest, and AreaWeighted #3701

Merged
merged 9 commits into from
Sep 9, 2020

Conversation

bouweandela
Copy link
Member

@bouweandela bouweandela commented Apr 28, 2020

This pull request implements lazy regridding for the Linear, Nearest, and AreaWeighted regridders.

Closes #3700

@stephenworsley
Copy link
Contributor

This certainly looks promising. It seems like something Iris ought to have and we deffinitely appreciate the effort to getting this in. We'll see what we can do about coming to a decision on this and getting this reviewed.

@JaroCamphuijsen
Copy link

@stephenworsley This would be very useful, is there a decision from the iris team already?

@trexfeathers trexfeathers added this to the v3.1.0 milestone Aug 21, 2020
@trexfeathers
Copy link
Contributor

We hope to give this the attention it needs for Iris v3.1

@bjlittle
Copy link
Member

@SciTools/iris-devs Why can't this be included for iris 3.0?

It's really going to massively help the ESMValTool community, and it's an all round win for the general community.

Sadly this PR has been sitting here accruing zero value for almost 4 months ☹️

@trexfeathers
Copy link
Contributor

SciTools/iris-devs Why can't this be included for iris 3.0?

I appreciate the frustration but we have already planned in as much as we can for delivering Iris 3.0 on time. We discussed this and several other items on 20/08, planned then allocated to 3.0 and 3.1 accordingly.

@bjlittle
Copy link
Member

@bouweandela You'll need to take this PR out of draft if you're keen to get this merged

@bjlittle
Copy link
Member

bjlittle commented Aug 24, 2020

@trexfeathers Begs the question, when is iris 3.1 planned?

(Apologies, I'm just being divisive. I don't expect you to answer that question, but we do need a more transparent release roadmap for the community... that's my main point here)

@bouweandela bouweandela marked this pull request as ready for review August 26, 2020 15:37
@bouweandela
Copy link
Member Author

The pull request is now ready for review @bjlittle. If there is anything I can help out with to get this across the finish line, please let me know!

@pp-mo
Copy link
Member

pp-mo commented Sep 2, 2020

lazy regridding for the Linear, Nearest, and AreaWeighted regridders

Really neat work !
We should have a good whatsnew entry to publicise it, though.

@bouweandela
Copy link
Member Author

Thanks @pp-mo! I just added a whatsnew entry. Would this be good enough, or should it explain more?

@pp-mo
Copy link
Member

pp-mo commented Sep 3, 2020

I just added a whatsnew entry. Would this be good enough

I think that's pretty much enough actually.
But looking at how it reads, I would link explicitly to the reference docs for the classes mentioned (see my suggested).
And, I think we should add a note about the lazy support in the docstrings of those classes : What you have said here about the AreaWeightedRegridder and here about the RectilinearRegridder is all fine+great, but only of use to devs as these are low-level components + don't appear in the reference docs. So, something similar where people can see it would be great !

lib/iris/cube.py Outdated
Comment on lines 4467 to 4470
* :class:`iris.analysis.UnstructuredNearest`,
* :class:`iris.analysis.PointInCell`,
* :class:`iris.experimental.regrid.ProjectedUnstructuredLinear`,
* :class:`iris.experimental.regrid.ProjectedUnstructuredNearest`.
Copy link
Member Author

Choose a reason for hiding this comment

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

I found several more regridding schemes and added them here. Does that look right, or were they missing for a reason?

Copy link
Member

@pp-mo pp-mo Sep 7, 2020

Choose a reason for hiding this comment

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

I think this is out of date.
But also, we don't want to commit to the experimental ones.
So maybe it should omit those, and amend to say "The regridding schemes in Iris currently include:" ...

Copy link
Member

Choose a reason for hiding this comment

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

Also.. if we extend this then the next section that you added "These regridding schemes support lazy regridding..." needs to change, as not all these schemes support it.

Copy link
Member Author

Choose a reason for hiding this comment

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

I removed the experimental regridders and changed the section in the user guide so it explicitly mentions which regridding schemes are lazy.

Copy link
Member Author

Choose a reason for hiding this comment

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

I did not add a section or mention of the UnstructeredNearest or PointInCell schemes to the user guide, because I would not feel qualified to write such a section and I also feel that it would be beyond the scope of this pull request.

@bouweandela
Copy link
Member Author

@pp-mo I added a section in the Regridding chapter in the user guide and a note about whether a regridding scheme is lazy or not to all regridders I could find. Does that look better or is it too much? Is there anywhere else you would expect to see documentation of the feature?

@pp-mo
Copy link
Member

pp-mo commented Sep 7, 2020

Is there anywhere else you would expect to see documentation of the feature?

No, I think this looks great really.
It's true, in my view, that we could do with taking some overly deep information out of the User Guide, possibly into the "technical papers" section. It has grown over time, and I personally wish we had a more minimal User Guide like "what everyone should know".
But that's a bit of a separate project. In the meantime, what you're adding here is not really long enough to justify a technical paper, and is actually not out of keeping with the existing User Guide.

@bouweandela
Copy link
Member Author

Thank you for reviewing again @pp-mo! If you require any further adjustments, please let me know.

@bjlittle
Copy link
Member

bjlittle commented Sep 8, 2020

@bouweandela We've just been experimenting with the GitHub actions/labeler to automatically label new pull requests, see #3819.

It appears to have failed for you PR (sorry about that!) which existed prior to #3819 being merged... so I was wondering whether you could rebase from master to pull in that PR to see if it makes a difference to the failure that we're seeing here in your PR.

Cheers! 😄

@bjlittle bjlittle modified the milestones: v3.1.0, v3.0.0 Sep 8, 2020
@bouweandela
Copy link
Member Author

I was wondering whether you could rebase from master to pull in that PR to see if it makes a difference to the failure that we're seeing here in your PR.

@bjlittle It looks like you deleted the labeler action again from the master branch, is this question still relevant? I suspect that the action failed because of actions/labeler#36 (comment).

@bjlittle
Copy link
Member

bjlittle commented Sep 9, 2020

@bouweandela Apologies for the carnage... we were having major problems with that GitHub action. So much so that it was basically failing on all PRs, which is very not cool.

So we pulled it out, since it's not stable, sorry for the flip-flopping.

@pp-mo Otherwise, it's totally okay for this PR to be merged with the labeler action failing - don't let that be a barrier to this PR not being merged

@pp-mo
Copy link
Member

pp-mo commented Sep 9, 2020

I think this is really awesome work @bouweandela ! 👍 💐
The care for transparency and user documentation is really appreciated, too.

My only remaining problem is with the whatsnew file : since we have totally changed the whatsnew contribution scheme, merging this will re-introduce a 'contributions folder' + mess it up.

But rather than make you jump through more hoops, I will merge this and post a new PR to fix that.

@pp-mo pp-mo merged commit 882889c into SciTools:master Sep 9, 2020
@pp-mo
Copy link
Member

pp-mo commented Sep 9, 2020

I will merge this and post a new PR to fix that.

See #3834

@bouweandela bouweandela deleted the lazy-regrid branch September 9, 2020 11:29
tkknight added a commit to tkknight/iris that referenced this pull request Sep 11, 2020
* master:
  Rework whatsnew into new scheme. (SciTools#3834)
  Lazy regridding with Linear, Nearest, and AreaWeighted (SciTools#3701)
  Iris readme minimal (SciTools#3833)
tkknight added a commit to tkknight/iris that referenced this pull request Sep 11, 2020
* master: (36 commits)
  Rework whatsnew into new scheme. (SciTools#3834)
  Lazy regridding with Linear, Nearest, and AreaWeighted (SciTools#3701)
  Iris readme minimal (SciTools#3833)
  Fix sphinx logger (SciTools#3832)
  whatsnew entry for dependencies (SciTools#3831)
  Added link to scitools.org.uk on the main index. (SciTools#3830)
  Updated Iris install instructions (SciTools#3817)
  Delete labeler.yml
  Delete label.yml
  Create label.yml
  Delete labeler.yml
  Update labeler.yml
  Update labeler.yml
  Add template collapsible markdown section (SciTools#3823)
  Requirements re-haul (SciTools#3812)
  Automate pull request labels (SciTools#3819)
  move whatsnew contributions (SciTools#3816)
  Parse with packaging version (SciTools#3815)
  Add "What's New" entries for unpinning Cartopy, Matplotlib, Proj (SciTools#3811)
  tidy issue templates (SciTools#3814)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Lazy regrid?
6 participants