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

Weights are returned in memory instead of being written to disk #3

Merged
merged 8 commits into from
Aug 21, 2020

Conversation

huard
Copy link
Contributor

@huard huard commented Aug 3, 2020

This sets factor=True and filename=None in the call to ESMF.Regrid, and then calls get_weight_dict to obtain a dictionary of weights in memory instead of writing the weights to disk.

The Regridder interface is changed to pass weights explicitly (weights replacing filename and reuse_weights=True).

Computed weights are saved to disk using a to_netcdf method.

Fixes JiaweiZhuang/xESMF#75
See JiaweiZhuang/xESMF#91

@raphaeldussin
Copy link
Contributor

raphaeldussin commented Aug 14, 2020

@huard I have a lot of cases using weights computed outside of xESMF and that I import into it using the filename argument. Can we maintain this functionality?

to be more specific, I'd really like to keep these arguments and their behaviors so that I don't have to update a bunch of notebooks that are currently tested in CI/CD with different versions of xESMF

@huard
Copy link
Contributor Author

huard commented Aug 14, 2020

I think it's maintained, I just changed the keyword parameter from filename to weights to acccount for in-memory weights. I
agree the PR would be stronger if it was backward compatible. What do you think of reinstating the filename argument, raising a DeprecationWarning and pass the filename value to weights ? Would that work for you ?

@raphaeldussin
Copy link
Contributor

yeah I think reinstating filename and reuse_weights and passing the value of filename to weights would work. Your weights are more generic and it solves problems/add capabilities but I'm gonna guess a lot of users (like myself) are now used to the filename/reuse_weights combination so I don't think we should make this go away. Keeping them in the API does not break anything, right?

@raphaeldussin
Copy link
Contributor

on a sidenote, I've been using pretty intensely the parallel ESMF_RegridWeightGen for large size problems that my workstation cannot handle. I was able to create weights for fairly large matrices (13000x13000 to 3000x43200) using 200+ cores and importing the computed weights in xESMF using the filename and reuse_weights arguments. I was thinking of putting a little chapter about this in the xESMF documentation since there are some "tricks" to get this working right.

@huard
Copy link
Contributor Author

huard commented Aug 14, 2020

Sounds good ! I'll only be able to look at this in a couple of weeks though, so if you want to move forward faster, please go ahead and update the PR.

@raphaeldussin
Copy link
Contributor

@huard I'll try to work on this off your PR while I'm in my ESMF streak

@raphaeldussin
Copy link
Contributor

PR pending on Ouranosinc/xESMF#1 that fixes the retro-compatibility issues of this PR

@jeffreysward
Copy link

Hello, I'm new to xesmf, and I'd like to take advantage of holding weights in memory. However, I'm currently getting the error TypeError: __init__() got an unexpected keyword argument 'factors' I think it's something simple and due to the fact I'm a little thrown by the relationship of this repo to the original xesmf at https://github.com/JiaweiZhuang/xESMF/ and which I should be following for updates.

@raphaeldussin
Copy link
Contributor

@jeffreysward xESMF is being moved to pangeo-data so that @JiaweiZhuang can get the help he needs with maintaining the package.

thanks for the bug report, @huard and myself should look into it shortly

@huard
Copy link
Contributor Author

huard commented Aug 27, 2020

@jeffreysward Which version of ESMpy is installed ?

I realize that the setup.py should specify which version of ESMpy is required.

@jeffreysward
Copy link

@huard it's version 7.1.0 from conda-forge. I chose this one as the quick remedy to the ImportError: Regrid(filename) requires PIO and does not work if ESMF has not been built with MPI support problem, but if there's a different workaround for that now, I'm not attached to that esmpy version for any other reason.

@raphaeldussin
Copy link
Contributor

I typically install ESMF/ESMPy with MPI support, else it does not work, properly, see here:

https://github.com/raphaeldussin/OM4p125_tideamp/blob/master/repro.yml

@huard
Copy link
Contributor Author

huard commented Aug 27, 2020

I believe the in-memory support in esmpy was added in 8.0.

@jeffreysward
Copy link

Okay, I've switched to esmpy v8.0.1 and successfully did a curvilinear bilinear regridding without creating a weights file (Although it still appears to set a default file name at some stage). Unfortunately, the MPI support creates peripheral problems on my cluster as in JiaweiZhuang/xESMF#102

@raphaeldussin
Copy link
Contributor

it defines a filename so we can be retro-compatible. I had issues with other circumstances with conda installed MPI on HPC. The hack is to recompile mpi4py from sources using the path to the MPI libs from your HPC system. See mpi4py

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Proposed new regridder save-load API in 0.3.0 (not backward compatible)
3 participants