-
Notifications
You must be signed in to change notification settings - Fork 37
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
Add ismip6 forcing testgroup #410
Add ismip6 forcing testgroup #410
Conversation
Hello @hollyhan! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:
Comment last updated at 2022-09-12 20:42:00 UTC |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hollyhan, this is amazing work! I went through processing ocean thermal forcing, basal melt, and surface mass balance all using this branch, and noted a handful of sticking points in my review. I have not yet gone through all the files, just any that gave me issues while testing it out. This was incredibly useful, and I'm now running the newest Thwaites mesh and optimization with UKESM forcing!
Also, I know you didn't ask for a review from me, but I needed to use this so I figured I might as well review it while I figured it out. :)
Thanks for this amazing new tool!
compass/landice/tests/ismip6_forcing/atmosphere/process_smb_racmo.py
Outdated
Show resolved
Hide resolved
compass/landice/tests/ismip6_forcing/ocean_thermal/process_thermal_forcing.py
Outdated
Show resolved
Hide resolved
compass/landice/tests/ismip6_forcing/ocean_basal/process_basal_melt.py
Outdated
Show resolved
Hide resolved
compass/landice/tests/ismip6_forcing/ocean_thermal/process_thermal_forcing.py
Show resolved
Hide resolved
compass/landice/tests/ismip6_forcing/ocean_basal/process_basal_melt.py
Outdated
Show resolved
Hide resolved
compass/landice/tests/ismip6_forcing/atmosphere/create_mapfile_smb.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
compass/landice/tests/ismip6_forcing/atmosphere/process_smb_racmo.py
Outdated
Show resolved
Hide resolved
compass/landice/tests/ismip6_forcing/atmosphere/process_smb_racmo.py
Outdated
Show resolved
Hide resolved
compass/landice/tests/ismip6_forcing/atmosphere/process_smb_racmo.py
Outdated
Show resolved
Hide resolved
compass/landice/tests/ismip6_forcing/atmosphere/process_smb_racmo.py
Outdated
Show resolved
Hide resolved
compass/landice/tests/ismip6_forcing/ocean_thermal/process_thermal_forcing.py
Outdated
Show resolved
Hide resolved
1. add/get different config options for input and output files and their paths 2. add a nested dictionary for loading different isimip6 forcing files 3. add a method that applies the MALI base SMB to the ismip6 smb anomaly field 4. fix how to add xtime variable; use `xarray.DataArray.dt` instead of `pandas.to_datetime`
Also set path to output data from config option
Also fix the dictionary list
So that the keys (file path and names) match the updated original ISMIP6 data repo
Previously, the ocean basal testcase had a config option for the remapping method. However, as we always want to use the "nearest neighborhood" method for this testcase, we decide to hard code the method in the main code "process_basal_melt.py" and remove the config option from the config file.
a6c35f1
to
52ed5bd
Compare
Thanks so much for testing the PR and for making really great suggestions for improving the testgroup! Thanks :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hollyhan, I tested today with HadGEM2-ES data and found a few things that still need to be fixed.
Please have a look and I'll test again once these changes are in.
compass/landice/tests/ismip6_forcing/atmosphere/process_smb_racmo.py
Outdated
Show resolved
Hide resolved
compass/landice/tests/ismip6_forcing/atmosphere/process_smb_racmo.py
Outdated
Show resolved
Hide resolved
Thanks for testing the PR so promptly, @xylar! I have turned in the changes. :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wonderful, @hollyhan! This has been a long process with a lot of back and forth. Thank you for bearing with it. I just reran my HadGEM2-ES tests and they all ran perfectly (in under 3 minutes total!).
I think this is ready for @matthewhoffman and @trhille to take a look at.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hollyhan , this looks fantastic! I did a cursory review again and I don't see any additional things you haven't already addressed. This has been a lot of work, but it has been very useful to be able to generate these forcings for any any mesh quickly, and adjust to updates and changes in a reproducible way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work, @hollyhan. I added a few minor comments, but I'm happy for this to be merged. I tested it end-to-end for all four tests and did a short forward run on the full AIS-4km mesh using the climatology forcings it produced.
compass/landice/tests/ismip6_forcing/atmosphere/create_mapfile_smb.py
Outdated
Show resolved
Hide resolved
compass/landice/tests/ismip6_forcing/ocean_basal/process_basal_melt.py
Outdated
Show resolved
Hide resolved
ds.close() | ||
|
||
# create a nested dictionary for the ISMIP6 original forcing files including relative path | ||
_file_obs = ["AIS/Ocean_Forcing/climatology_from_obs_1995-2017/obs_thermal_forcing_1995-2017_8km_x_60m.nc"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible to make the 'Ocean_Forcing' part of this case-insensitive? It's a little awkward to have to have both an 'Ocean_Forcing' and an 'Ocean_forcing' directory when processing the obs and 2300 files, and it's not immediately apparent to the user why the ocean_thermal_obs
case doesn't work when the ocean_thermal
case does.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@trhille, this is a good point, and it would be ideal to have a case-insensitive directory for the ocean forcing files. The observational climatology file was originally only downloadable from the ISMIP6-2100 GHub endpoint, which is why I put the obs file in the same directory name Ocean_Forcing
as that for the 2100 files. But the ISMIP6 committee has recently uploaded the obs data to the 2300 Ghub endpoint as well. I can see how to make the directory for the obs file case insensitive depending on which period_endyear
is chosen in the config file in the following PRs I will create after merging this one.
816c401
to
867bab7
Compare
Thanks so much for catching the silly typos with careful inspection, @trhille! I fixed all of them and updated the PR! |
Hi @xylar, @trhille, @matthewhoffman, Thank you all so much for helping me through this PR, which involved multiple rounds of thorough reviews and giving constructive feedbacks! I've updated the PR with the last comments provided by Trevor above. |
Congratulation, @hollyhan! |
Yes, awesome work @hollyhan! This is such a great tool! |
This PR adds a test group to COMPASS that processes the ISMIP6 forcing data provided by the ISMIP6 experimental protocol that target the upcoming IPCC AR6 assessment (https://www.climate-cryosphere.org/wiki/index.php?title=ISMIP6-Projections-Antarctica). Processing includes remapping the original data onto MALI mesh and renaming variables so that the output data can be used to force MALI. In the test group, three test cases have been added:
atmosphere
,ocean_basal
andocean_thermal
.Original version of this PR is #377, which has been closed in favor of this most updated version, which adds a step to the
atmosphere
test case that processes the modern climatology (1995-2017) of the RACMO data (https://tc.copernicus.org/articles/12/1479/2018/) and data and applies a climatology correction to the ismip6 SMB anomaly data.