-
Notifications
You must be signed in to change notification settings - Fork 45
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
Plumber updates #262
Plumber updates #262
Conversation
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.
The changes for PLUMBER appear to parallel the existing code for NEON, so they seem logical and simple to follow.
Before approving, we probably need to run tests. I have never worked on a CDEPS PR, so I'm not familiar with relevant testing, nor with the merging process (though it's likely similar to CTSM's, RTM's, MOSART's, etc.). But also I doubt that I have the necessary permissions to merge a CDEPS PR...
From meeting with @ekluzek
|
A note on somethings we need to do before this is asked for merge:
|
Agreed in today's ctsm software meeting: |
Per conversation with Erik, we can remove the files listed in PLUMBER2 user mod directories because these will be implemented in another PR (#277 ); those do not need to be moved to CDEPS. However, we do need to implement the dtlimit used for these various streams specifically for PLUMBER-- hence the placeholder values that I still need to ensure work properly for changing dtlimit when CLM_USRDAT_NAME is set to PLUMBER. Variables in those user mod directories that are duplicated in the CDEPS stream can be removed once this PR is merged in. |
The |
This PR introduced CLM_USRDAT_NAME as PLUMBER2 instead of PLUMBER, so I will update that now. |
@TeaganKing I want to confirm that I understand. Also, a note to myself: The checklist points out that I need to generate a baseline. |
ESCOMP/CTSM#2406 is very much still in progress, and there are going to be a few changes to ESCOMP/CTSM#2485 still as well (I'll do this within the next few days). What exactly is being tested with the aux_cdeps tests? I personally tested this one just by doing an xmlchange to set CLM_USRDAT_NAME to PLUMBER2, building the case, and checking the input files. |
Ok, based on this information, I think I could go ahead and submit aux_cdeps with #262 with ctsm from master (I will try ctsm5.2.007 which is the current latest). |
I tried and failed to generate a baseline using the latest ctsm paired with cdeps1.0.38, i.e. the same cdeps that I see in @TeaganKing's branch: I also tried and failed to generate a baseline using the latest ctsm paired with cdeps1.0.34, i.e. the default cdeps for ctsm5.2.008: The former seems less surprising, if e.g. there are incompatibilities between ctsm5.2.008 and cdeps1.0.38. The latter though means that I have a problem with aux_cdeps (environment or other?) or that aux_cdeps has a problem (in which case it should fail for others, as well). @TeaganKing at this point I will need help from @ekluzek with this. I will raise the issue at Monday's stand-up. |
I encountered the same problem this morning even with aux_clm and ctsm_sci. This helped me realize that the problem may be as simple as setting an account number that hasn't expired. I will try this again today or tomorrow. UPDATE 1: UPDATE 2:
UPDATE 3: |
@TeaganKing two updates:
|
In case it helps, here's a list of tests that PASS versus FAIL:
As far as I can tell, the PEND failures report the same error as the FAIL in this list:
|
My quick look at the above lists suggests that SATM tests PASS and DATM tests fail. |
Thank you for running these tests and clarifying the 2nd checkbox item! Regarding actually running the PLUMBER case, we don't have |
Hi @jedwards4b and @billsacks , In a discussion with @wwieder and @ekluzek we decided that it would be best to include an xml variable The alternative option that we thought of would be to leave the input files as adjustments to the namelists that could be done in the usermods directories; however, this would be a different (and hence potentially confusing to users) implementation compared to the neon site input file implementation. |
Thanks for explaining this, @TeaganKing . I appreciate your laying out the details as well as the alternative that you considered - that's very helpful context. I feel like I'm missing something, because I'm not seeing where <file first_year="$DATM_YR_START" last_year="$DATM_YR_END">$DIN_LOC_ROOT/atm/datm7/CLM1PT_data/PLUMBER2/${PLUMBER2SITE}/CLM1PT_data/CTSM_DATM_${PLUMBER2SITE}_2010-${DATM_YR_END}.nc</file> It makes sense to me to have an xml variable for this purpose. It seems like the ideal situation would be to have consistency between the different possible datm modes... something like always using this |
Yes, that's precisely where it will be implemented (replacing the 2010 in that example). Okay, thanks for the feedback! |
Hi @jedwards4b and @billsacks , would one of you be able to review this PR? The |
Thanks @TeaganKing ! I'm taking a look at this now.... |
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.
@TeaganKing this is great, thanks for working on this and getting it figured out. I appreciate your sticking it out even though it got difficult.
I ask for a small change, but marking as approved so you don't have to wait for me to reapprove.
I also would've wanted you to add testing around this in CDEPS, but realized the reason for that is that CLM_USRDAT_NAME, NEONSITE, and PLUMBER2SITE are all defined in CTSM. I think we should move them to here in CDEPS so that we can do testing for them in CDEPS and not just in CLM. I'll make an issue around doing that here. This would be a longer term code health/testing improvement type of thing. I suspect it would've been a lot easier to do a lot of the testing here if we could test the CDEPS side for NEON and PLUMBER2 with compsets without CLM.
@TeaganKing in terms of things to do it looks like aux_cdeps passes, have you also run the latest in CTSM to ensure it works? Just want to make sure all the boxes are ticked off..
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.
Thanks for your work on this @TeaganKing ! And thanks for running the aux_cdeps
testing!
This looks good to me, though I have one comment below about some logic that I don't understand that I think at least warrants a comment.
My understanding is that this is ready to merge from your perspective; is that right? If so, once you respond to / address my comment, I can merge it.
@ekluzek I will open up an issue for testing these if they are moved into CDEPS. In regards to running the latest in CTSM, are you just proposing pulling in the most recent CTSM changes, replacing the cdeps directory with this PR, and then running aux_cdeps? |
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.
Thanks again for your careful work on this @TeaganKing, and for sticking with the back and forths this morning !
I am marking this as approved. My sense is that there might be some additional testing that @ekluzek is asking for... I'm not following that carefully... if you let me know once you feel that this is ready to merge I'll go ahead and merge it.
Thanks for reviewing this @billsacks ! I'm also not totally following the testing suggestion from @ekluzek -- Erik, do you mind explaining that a bit more? |
Sorry I just wanted to make sure we had all the boxes checked off here: Namely making sure this CMEPS code works in a branch of CTSM where you are bringing this CMEPS update into CTSM. And run CTSM for a few PLUMBER2 sites to make sure it works fine inside of CTSM. This is where if we could add PLUMBER2 tests to CDEPS -- you'd just have to run aux_cdeps. But, because we can't we need to make sure they work from within CTSM, for running PLUMBER2 sites in a CTSM checkout. Does that make sense now? |
Okay. So I have run this for a few PLUMBER2 sites and it worked fine inside of CTSM-- but that required using the changes from both of the CTSM PLUMBER-related PRs. |
Perfect, that's what I wanted to make sure had been done. It's normal for CTSM work to also depend on an external like CDEPS. Once, you can show the thing both works with the new CDEPS and other CTSM changes -- and doesn't break other CDEPS cases, we make the CDEPS tag. Then you can point to it in your CTSM PR, which still probably will take work to come into the CTSM side. So I checked off all the checkboxes now. @billsacks please press the merge button! Thanks everyone! |
Copying an email from @fischer-ncar: In some pre-prealpha testing I ran into some unexpected answer changes from #262 (cdeps1.0.47) RMS atmExp_So_u10withGust 6.8157E+00 NORMALIZED 2.6838E+00 For MOM I'm seeing this kind of answer change. RMS Med_aoflux_ocn_So_u10withGust 6.3118E+00 NORMALIZED 2.9113E+00 I'm not too overly worried about the answer changes since I'm only seeing them with coupler fields. But worried enough that I should make people aware of it.
Chris |
An update about the possible change in answers from this PR. @fischer-ncar in doing more testing found that this isn't the source of the answer changes. So he's doing more testing to find the cause. |
This will address #248 In order to implement PLUMBER capabilities