MPAS-Dev / MPAS-Tools

MPAS Tools Repository
Other
37 stars 63 forks source link

Update CISM script to be compatible with python 3 #444

Closed hollyhan closed 2 years ago

hollyhan commented 2 years ago

This merge

hollyhan commented 2 years ago

@matthewhoffman @xylar - This is ready to review. Thanks :)

xylar commented 2 years ago

@matthewhoffman or @hollyhan, does either of you have an example CISM file handy that I could use to test this on?

matthewhoffman commented 2 years ago

@hollyhan & @xylar , this looks great - thanks both of you for working through the details of updating this.

@trhille , do you have a simple regular grid GIS or AIS dataset that could be used for testing this regridding script? We call it 'cism' format here, but we don't actually use CISM files anymore.

xylar commented 2 years ago

@matthewhoffman and @trhille, @hollyhan sent me a file to work with so I'm set.

xylar commented 2 years ago

We include the create_SCRIP_file_from_MPAS_mesh.py in the mpas_tools conda package. Would it be helpful to include this script, too? If so, it should be added below here: https://github.com/MPAS-Dev/MPAS-Tools/blob/master/conda_package/setup.py#L64

@matthewhoffman, what do you think?

matthewhoffman commented 2 years ago

@xylar , now that you mention it, yes it would be useful to include this as well in mpas-tools package. The use case I am thinking of is when we need to do conservative remapping from ice sheet source datasets to MALI meshes in compass. We have historically just done these as one-offs but it would be valuable to have the ability within compass. Should we add that package change to this PR in an additional commit? (after the script name change)

xylar commented 2 years ago

Should we add that package change to this PR in an additional commit? (after the script name change)

Yes, I think that makes sense. No need to keep it separate.

xylar commented 2 years ago

Thanks for the updates, @hollyhan. This is coming along really well. I had a few suggestions above, mostly just because these are the ways we handle "boolean" flags like --plot most of the time.

I'll re-test as soon as I can. In the meantime, you can feel free to rename the script unless @matthewhoffman would prefer you wait.

xylar commented 2 years ago

I'm not sure how to start conversation on the new page

@hollyhan, to start a conversation about a part of the code, you go to "Files changed" at the top of the page, scroll down to the code changes, move your mouse over to the left margin (before the line numbers) so you see a blue plus sign, and select the line(s) you want to discuss. Then, a small conversation window will pop up. You can use the "Insert a suggestion" button (a page with a +/- on it) to make suggestions about changes to the code like I've been doing.

Some other handy code formatting tips for GitHub are here: https://guides.github.com/features/mastering-markdown/ These also work (pretty well) on Slack.

xylar commented 2 years ago

@hollyhan, I think your last commit added a new file named create_SCRIP_file_from_planar_rectangular_grid.py but didn't delete the one with the old name. Could you do:

git rm mesh_tools/create_SCRIP_files/create_SCRIP_file_from_CISM_mesh.py
git commit
git push ...

That should take care of it.

xylar commented 2 years ago

@hollyhan, is it okay if I push a change to this branch that would add this file to the conda package? I don't want to step on your toes and you would need to do:

git fetch --all -p
git reset --hard hollyhan/MPAS-Tools/fix_create_SCRIP_file_from_CISM_mesh

to get my changes after I make them. In that process, any changes you have made that aren't yet on your rote branch would be lost. (There are fancier things we can do to handle this more elegantly, but we haven't had a chance to discuss them yet.) That's why I'm checking with you first.

matthewhoffman commented 2 years ago

This progress looks good to me. I'll do a final review after the @xylar 's last couple comments are resolved.

hollyhan commented 2 years ago

Hi @xylar , yes! Please go ahead and push the change to the branch - you won't be stepping on anyone's toes! Thanks for checking with me.

xylar commented 2 years ago

@hollyhan, the changes I made are on my work laptop so I'll push them on Monday. Thanks for the latest update.

xylar commented 2 years ago

@hollyhan, I just pushed my changes. I'm going to re-test one more time.

@matthewhoffman, I think you could also re-review at this point.

hollyhan commented 2 years ago

Thanks so much @xylar! And sounds good about describing the steps in another venue. Thank you! :)

xylar commented 2 years ago

@hollyhan, let me know if you've run the extra steps (ESMF_RegridWweightGen or ncremap) that @matthewhoffman mentioned and if they were successful. If so, I'll merge.

hollyhan commented 2 years ago

@matthewhoffman and @xylar - I have tested the resulting remapping through ncremap and checked that the results are correct for both rank 1 and rank 2 scrip files generated using this updated code.

xylar commented 2 years ago

Great! Merging...

xylar commented 2 years ago

Thanks again @hollyhan and @matthewhoffman!

hollyhan commented 2 years ago

Thank you!! @xylar @matthewhoffman