Skip to content

Conversation

@davidhassell
Copy link
Collaborator

Fixes: #222

@davidhassell
Copy link
Collaborator Author

Hi Sadie - it'd be great if you could cast your eye over this.

The main elements are the change to the cf.Field.regrid[sc] APIs to allow a previously created RegridOperator (defined in cf/regrid/regridoperator.py) instance to define the regridding weights. By default the regridding weights are calculated afresh for every regrid.

I took this opportunity to move the regridding methods from the Field object to their own module (cf.regrid.utils), but that's just house keeping, so no real change, there.

Whilst I have done some work on the regrid[sc] docstrings, I am aware that they are fairly shabby in places. I think that this can be tackled separately - I'll raise a new issue for that.

Thanks,
David

@sadielbartholomew
Copy link
Member

Thanks David for the reviewing guidance which all seems very clear. I will review later today.

I took this opportunity to move the regridding methods from the Field object to their own module (cf.regrid.utils)

Excellent idea. I'd also be keen to move out other related aspects if we can find any because the size of the cf Field module is enough that e.g. I have had to disable line numbering with emacs as it makes it to slow for practical development (though in fairness with the extensions I use anything above 5000 means it is best to turn them off)!

@davidhassell
Copy link
Collaborator Author

Thanks, Sadie. I'm all for reducing the size of field.py in that way.

@davidhassell davidhassell added this to the 3.10.0 milestone Jun 9, 2021
@sadielbartholomew
Copy link
Member

I'm all for reducing the size of field.py in that way.

Excellent, I will look out for opportunities to do so.

I have just observed another indicator that the module is larger than it should be, in that GitHub gives up on syntax highlighting after about ~10,000 lines into a file (or a Python one at least)!

Copy link
Member

@sadielbartholomew sadielbartholomew left a comment

Choose a reason for hiding this comment

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

Excellent, performance much improved as per the issue at hand and nothing has broken with the update and refactor as far as I can tell. I only have a few typos to point out (and they can be addressed at a future time if you wish).

davidhassell and others added 3 commits June 10, 2021 09:11
Co-authored-by: Sadie L. Bartholomew <sadie.bartholomew@ncas.ac.uk>
Co-authored-by: Sadie L. Bartholomew <sadie.bartholomew@ncas.ac.uk>
Co-authored-by: Sadie L. Bartholomew <sadie.bartholomew@ncas.ac.uk>
@davidhassell
Copy link
Collaborator Author

Thanks, Sadie. All points addressed as per your suggestions (apart from the alphabetical ordering of imports which is best left to robots rather than me!)

@davidhassell davidhassell merged commit 56e4af2 into NCAS-CMS:master Jun 10, 2021
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.

Remembering weights between consecutive regridding operations

2 participants