-
Notifications
You must be signed in to change notification settings - Fork 7
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
Fix masking of 3D MPAS-Ocean variables #270
Conversation
CI seems to be failing for reasons unrelated to my one tiny change. |
It looks an issue with Codecov for CI to fail. @tomvothecoder could you please help with this? |
@chengzhuzhang, the other thing that we may need to modify is that we are renormalizing with a threshold of 0.0 right now: In a quick test, the existing threshold of 0.0 seems to be okay at least for the depth slices I looked at. |
@xylar It looks like the threshold=0.0 only applies to sea-ice variables. I think based on some earlier experiment, using 0.0 worked okay for sea-ice. and to increase it to a higher value, e.g. 0.05 will mark more grids (5%) as missing. |
d231c3a
to
d7d20bd
Compare
Rebased with latest |
Thank you for a quick fix! |
@chengzhuzhang, all the calls I see to mpas.remap don't include the threshold flag, meaning they all default to 0.0. Could you clarify where the 0.05 value you refer to comes from? I don't see it. |
I actually think it's the opposite of what you said: there's a 5% threshold for sea ice variables: e3sm_to_cmip/e3sm_to_cmip/mpas.py Line 235 in f816e64
and 0.0 for the ocean variables (if they were properly masked). Except that add_si_mask seems never to be used. |
We should remove the Just to clarify, I only tested with The reason I said the threshold for re-nomalization only applies to sea-ice is based on the ncremap command, where no re-normalization flag (--rnr_thr) is applied to ocean variables: e3sm_to_cmip/e3sm_to_cmip/mpas.py Lines 99 to 119 in f816e64
|
Oh, I see! That's another bug! And needs to be fixed here. But I won't be able to do it because I'm on vacation for the next 2 weeks. |
thanks for the heads-up, we can test and finalize this after you return. Enjoy the vacation! |
@chengzhuzhang, I have got rid of the unused |
Is this PR related to #255? |
@tomvothecoder, I don't believe so. |
@TonyB9000, any update on you having access to LCRC? |
@xylar I am logged into chrysalis (via the new "Duo Mobile" MFA). I recall having problems trying to fetch your changes for testing here (some permission issue). This could be my fault (git setup), but I believe I've pushed/pulled changes to my "datasm" codes from chrysalis before - or else I was hallucinating... Just now, I issued:
So I am going to conduct the "Adding a new SSH key to your GitHub account" instructions, to see if I can get that straightened out. |
@TonyB9000, yes, it sounds like your ssh keys aren't set up right for LCRC on GitHub. |
@xylar It seems I have the keypair
And I just added it to my git profile - but I have no idea what to enter for a passphrase (I tried my acme1 keyphrase and it just keeps asking . . . I guess I need to generate a new one. |
Yes, generate a new one. It's a pretty big pain to have a password because you have to enter it dozens of times if you do a recursive checkout. I personally think it's safe to generate a new one without a password. |
@xylar OK, I can now fetch and see (on chrysalis):
How should I proceed from here? I assume I want to create a conda env and "pip install" something... |
@TonyB9000, the branch name is at the top of the PR here, |
@TonyB9000 I'm copying the steps from Slack DMs:
Then you can install to the environment that you intend to test this new feature. |
@chengzhuzhang Thanks Jill. I hope there is also a way to verify the presence/absence of the problem. We have 400 datasets we need to regenerate - I believe. I can produce that list. I am a bit concerned when I see
But "git branch" lists
I have abandoned the branch "consolidate_cmor_setups_and_logging" in favor of two branches, "consolidate_cmor_setups_273" and "redesign_logger_274". After Tom's review, I merged "consolidate_cmor_setups_273", then discovered I had missed a few stragglers, and opened/pushed/PR's "missed_cmor_consolidations". On chrysalis, do I need to "fetch" those other branches? And how would I separate my "master" from xylar's? |
@TonyB9000 I'm not sure if I understand your question. But for testing this PR, you can just install this branch |
@chengzhuzhang OK, I created the env "dsm_test_xylar_e2c", pip installed both datasm (so I can access my auxiliary tools). Then I cd'd to my gitrepo/e3sm_to_cmip, did "git checkout" of Xylar's branch:
Then I pip-installed this branch. I can now run the "end-to-end" script (Operations/5_DatasetGeneration/Alt_Process/dsm_generate_CMIP6.sh) on some selected set of v2 and v2_1 datasets - where I happen to have the corresponding NATIVE data in the warehouse. I'll have to see what native v2 data exists. My real question is, when I decide to switch back to my master branch, will I get my original master, or the one that reads
(BTW: Turns out I only have v2_1 and some v3 native data, and of the v2_1 only 1pctCO2 and amip. The amip won't do us any good here (no MPAS) so I will need to test/generate some v2_1/1pctCO2/ocean stuff. I suppose a single year is sufficient for testing. But we should copy over some v2 and v2-NARRM as well, I suppose.) |
@chengzhuzhang These 10 cmip6 sets:
All rely upon the one native set I have on hand:
So I will run a 1-year test for each and we can examine the results. |
@xylar @chengzhuzhang We have completed 1 year each of the above 10 v2_1 1pctCO2 Omon CMIP6 datasets. The output files are in (chrysalis):
where "*" is any one of the 10 Omon variables listed above. The only unusual log output was for
NOTE: As this version of e2c has not had the logfile consolidation work applied, 8 of the cmor logs are in
Two are in
The more verbose e2c logs are in
|
@TonyB9000, the temp file looks like what I was hoping for so I think you did things right. Unfortunately, it didn't fix the problem. |
@xylar (Yes, I did the cherry-pick thing - forgot the details). Sorry it didn't work. Let me know what else I can do, as soon as you're ready. |
@TonyB9000, could you try one more time with the |
If that doesn't work, I'm going to need to ask Charlie for some help. |
@xylar Done. Files should be readable:
|
Thanks @TonyB9000! That also did what I wanted but didn't fix the problem. I'm going to experiment some more over the weekend and I'll ask for help from you or Charlie or whoever once I have a better idea what could be going on. |
@xylar Charlie may know - I an clueless egarding cmor/xarray internals... Cheers! |
Keeping an existing attribute seems to mean that NaNs don't get replaced with the fill value as they should.
33a25f8
to
e332f93
Compare
The problem turned out to be that I was keeping the In any case CMOR then changes the fill value to its preferred value down the road. The issue with NaNs is that aren't fill values (and maybe even using NaNs as the fill value) is that NCO then tries to renormalize them and gets NaNs everywhere that even overlaps a masked source cell. |
@TonyB9000, can you do the following?
I'm assuming your name for my remote is Could you then rerun the workflow on all variables as before? I want to make very sure the fix is working before we redo a bunch of ocean 3D variables. |
@xylar Great! I'll work on that this morning. |
@xylar Minor git issue: The first command went well:
Then
The second command complained:
Trying
Sorry for being a gitiot... |
@xylar Ahh I got it. Type I guess I want "3d-masking" not "e3-masking". |
Yes, just a typo. I just saw that, too. |
@xylar So far, the only complaint was this one that returned (from cmor) for hfsifrazil)
(I am not fond of the cmor log-output format.) Test runs are completed. The e2c logs are in
and
(permissions updated.) |
Thanks @TonyB9000! Here are the plots from the latest processed files: Everything looks great to me! I think this is ready to merge unless there are further concerns. (We don't want to include the changes to keep the temp files. That was just for debugging.) |
@xylar This is great! I'll perform the merge and (eventually) rebase my other branches. I can simply change the variable "retain_temp_files" to "False" in the code before merging. That way, future testing need not recall how to log the names of the given tempfiles. |
Thanks @TonyB9000! Now the hard work begins. Let me know if I can be of help there. |
@xylar "Hard work for the processors". For me, almost everything is automated. I take the list of 400 v2/v2_1 Ocean 3D-var CMIP6 dataset_ids (already broken down to 80 v2_1, 320 v2), and feed them to "dsm_generate_CMIP6.sh", changing "TEST" to "WORK" so that all the years are generated and the outputs are moved to the warehouse in preparation for publication (and the dataset status files are automatically updated). It may take a week or more to run, but all in background. Meanwhile, we have the other 917 v2_1 CMIP6 datasets to publish, and we have just now "established" (not yet tested) the new methodology for publishing E3SM datasets from ANL/chrysalis, So everything is lining up OK. |
I'm glad it's not an overwhelming amount of human time, at least! |
@xylar (at your leisure) Could you check one final "test set" of thetao: infile = /lcrc/group/e3sm2/ac.bartoletti1/tmp/tmp7n6ee9w5 I rebased my branch of "redesign_logger_274" to reflect your changes to ocean masking (merged to master), and then ran a test with the revised logging. I want to ensure I have not screwed anything up... |
@TonyB9000, yep the masking still looks good in |
@xylar Awesome! Thanks. |
Description
We have discovered that 3D ocean variables have not been correctly masked following #155. The review of that PR focused entirely on correctly masking sea-ice output and we missed the impact it had on 3D ocean variables, which was that is removed manual masking after remapping and assumed (incorrectly) that ncremap was handling the masking and renormaliztion itself. Instead, the 3D fields were being set to zero (not the
_FillValue
) below bathymetry, leading to fractional values after remapping, as reported in E3SM-Project/E3SM#6546Checklist
If applicable: