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.

Sign up for GitHub

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

Jump to bottom

Refactor lazy_regrid.py for wflow diagnostics #2024

Merged
merged 13 commits into from
Feb 18, 2021
Merged

Refactor lazy_regrid.py for wflow diagnostics #2024

merged 13 commits into from
Feb 18, 2021
+61 −116

Conversation

SarahAlidoost
Copy link
Member

@SarahAlidoost SarahAlidoost commented Feb 12, 2021 •
edited by bouweandela
Loading

Description

Before you get started

Checklist

It is the responsibility of the author to make sure the PR is ready to review. The icons indicate whether the item will be subject to the 🛠 Technical or 🧪 Scientific review.

New or updated recipe/diagnostic:

/diagnostic.html#additional-dependencies)

To help with the number pull requests:

@SarahAlidoost SarahAlidoost marked this pull request as ready for review February 15, 2021 08:28
@SarahAlidoost
Copy link
Member Author

@ESMValGroup/tech-reviewers this PR is ready for review. Thanks in advance.

Peter9192
Peter9192 requested changes Feb 15, 2021
View reviewed changes
Copy link
Contributor

@Peter9192 Peter9192 left a comment

Choose a reason for hiding this comment

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

Hi Sarah,
Good call! It seems as though you've accidentily included some changes from your local working directory, or is this intentional (and can you explain why it's needed in that case?)

Did you also compare the performance?

@SarahAlidoost
Copy link
Member Author

Hi Sarah,
Good call! It seems as though you've accidentily included some changes from your local working directory, or is this intentional (and can you explain why it's needed in that case?)

Hi Peter, thank you. These changes are intended to make the recipes more realistic regarding their applications, for more details please see my response to your comments on each change.

Did you also compare the performance?

Do you mean comparing the performance of esmvaltool.diag_scripts.hydrology.lazy_regrid and esmvalcore.preprocessor.regrid ?

valeriupredoi
valeriupredoi reviewed Feb 15, 2021
View reviewed changes
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.

am just crashing this party here because I am curious if you have a resource comparison between your implementation and the new iris lazy regrid implementation - don't have to be exact, but some rough numbers would be interesting to hear 👍

esmvaltool/diag_scripts/hydrology/wflow.py
import numpy as np
from osgeo import gdal
from esmvalcore.preprocessor import regrid
import iris
Copy link
Contributor

Choose a reason for hiding this comment

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

I am in awe about how Codacy is not complaining about this import order 😁

Copy link
Contributor

Choose a reason for hiding this comment

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

Me too, but I already asked Sarah to check something like that lately and it turns out I was the one that's wrong... This one is even more strange though!

Copy link
Member Author

Choose a reason for hiding this comment

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

hmmm, the prospector did not complain about it, and the pylint in vscode says: third party import "from esmvalcore.preprocessor import regrid" should be placed before "import iris"pylint.

Copy link
Contributor

Choose a reason for hiding this comment

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

it's possible prospector didn't run the pylint bit and the CI let it pass - it does that from time to time when there are lots of commits

Copy link
Contributor

Choose a reason for hiding this comment

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

looks better now 👍

@Peter9192
Copy link
Contributor

Do you mean comparing the performance of esmvaltool.diag_scripts.hydrology.lazy_regrid and esmvalcore.preprocessor.regrid ?

Yes, just like @valeriupredoi asked. I think the functions should be roughly the same, but I was also just curious

@SarahAlidoost
Copy link
Member Author

am just crashing this party here because I am curious if you have a resource comparison between your implementation and the new iris lazy regrid implementation - don't have to be exact, but some rough numbers would be interesting to hear

OK, I will do it.

@SarahAlidoost SarahAlidoost mentioned this pull request Feb 17, 2021
Closed
@SarahAlidoost
Copy link
Member Author

@Peter9192 and @valeriupredoi good that you both asked me to check the performance. Running the recipe with 10 years of data fails due to memory error. This is still an open issue in iris . Therefore, we decided to keep the lazy_regrid script. I refactor the script based on the iris documentation on Regridding Lazy Data. Comparing the performance is shown in issue #2025. (cc @bouweandela)

@SarahAlidoost SarahAlidoost changed the title Replace lazy_regrid.py with esmvalcore.preprocessor.regrid for wflow diagnostics Refactor lazy_regrid.py for wflow diagnostics Feb 17, 2021
bouweandela
bouweandela reviewed Feb 17, 2021
View reviewed changes
Copy link
Member

@bouweandela bouweandela left a comment

Choose a reason for hiding this comment

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

Looks good to me @SarahAlidoost! Just some suggestions for further cleanup.

@SarahAlidoost
Copy link
Member Author

@Peter9192 and @valeriupredoi could you please approve the PR or let me know if there are any concerns? thank you.

valeriupredoi
valeriupredoi approved these changes Feb 18, 2021
View reviewed changes
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.

nice work @SarahAlidoost 🍺

esmvaltool/diag_scripts/hydrology/wflow.py
import numpy as np
from osgeo import gdal
from esmvalcore.preprocessor import regrid
import iris
Copy link
Contributor

Choose a reason for hiding this comment

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

looks better now 👍

Peter9192
Peter9192 approved these changes Feb 18, 2021
View reviewed changes
Copy link
Contributor

@Peter9192 Peter9192 left a comment

Choose a reason for hiding this comment

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

Nice work Sarah, looks nice and clean now :-)

@valeriupredoi
Copy link
Contributor

three approvals, it's like Christmas come early 😆

@valeriupredoi valeriupredoi merged commit 694c094 into master Feb 18, 2021
@valeriupredoi valeriupredoi deleted the refactor_hydro_recipes branch February 18, 2021 12:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bouweandela bouweandela bouweandela approved these changes

@Peter9192 Peter9192 Peter9192 approved these changes

@valeriupredoi valeriupredoi valeriupredoi approved these changes

Assignees
No one assigned
Labels
diagnostic enhancement in technical review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Refactor lazy_regrid.py for wflow diagnostics
5 participants
@SarahAlidoost @Peter9192 @valeriupredoi @bouweandela @jvegreg