modify_fsurdat: Add dom_cft as alternative user-choice to dom_nat_pft #1615
Conversation
slevis-lmwg
added
enhancement
There was a problem hiding this comment.
@slevisconsulting this is actually something that we might want to be consistent with subset_data. In subset_data, you can pick either a crop or a nat-veg with the same --dom_pft option, by using PFT numbering. See the issue #1606 which goes over this, and if you go with that you'd need to do the same here. Here you have one way to set a Crop PFT and another for a natural-veg PFT. We likely want to have this consistent in the way we handle it. An advantage of the former is that you only need one option rather than two, and I think that's helpful.
The other thing I noticed is that there are a few magic numbers in here (15, 79, and 12 is what I saw), these should be changed to variables so the context of the meaning behind them is given.
And finally I didn't see the change added to a unit test and if you can it would be good to do that.
Thanks for putting this together. Feel free to reach out to me to go over it.
|
Thank you very much for your work on this @slevisconsulting . I don't feel a need to review it. |
- Removed a bunch of "magic" numbers - Consolidated two options (dom_nat_pft and dom_cft) into one (dom_plant) - Not in Erik's review, but I added some new logger.info lines to indicate the progress of a run when using --verbose
Thank you for your comments, @ekluzek |
|
Python tests PASS |
There was a problem hiding this comment.
This looks good to me. You addressed the suggestions I had earlier.
I'd still like to meet with you to go over it a bit. It would help me to understand how the testing for this works. There is one comment I made about a magic number, but I'm not sure it's something we can really change and might be OK as it is...
Resolved conflicts: doc/ChangeLog doc/ChangeSum python/ctsm/test/test_unit_modify_fsurdat.py
slevis-lmwg
added
the
PR status: ready
|
I linked the dev081 tests to dev082. |
|
@slevisconsulting OK is this ready to go from your perspective? If so I'll do a final review and approve it and then make the tag. Also are there new clm_pymod tests or are answers different for them than before? If so we should link each specific test directory, rather than the top level, and then remove the softlinks for the clm_pymod tests and then rerun them. Could you do that? |
I think this is ready. |
|
@negin513 this is a really valuable comment here. I agree that tools are
easier to understand if the user interfaces are similar between them. I
actually made a similar comment at the top of...
#1615 (review)
But, since I didn't make a checkbox out of it, it's easy to miss. So I
agree with your comment. I think based on where we are at with this
specific PR, we should go ahead as is, but I'll make an issue to address
that for a future one that I'll assign to @slevisconsulting. I think we
should have a few of us meet next week to discuss the user interfaces of
the tools to make sure we are building them similarly. This also tells me
that I should use checkboxes more often. I have a love/hate relationship
with them, but I'm realizing they are important to ensure things don't get
lost. :-) @billsacks is the King of using them, and I think the one that
originally endorsed the idea. I think he's right they really do help.
Also @negin513 when I refreshed the issue in github I didn't see your
comment, so I'm sending this from my email. I haven't figured out why I
didn't see your comment in github itself.
*Erik Kluzek, (CGD at NCAR)*
*National Center for Atmospheric Research*
*Boulder CO, *
*(off) (303)497-1326 (fax) (303)497-1348*
*(skype) ekluzek (cell) (303)859-0183*
*(github) ekluzek **Pronouns: he/his/him **(*Why do I list my personal
pronouns? <https://www.mypronouns.org/what-and-why/>)
*------------------ Home page ------------------------*
* https://staff.ucar.edu/users/erik
<https://staff.ucar.edu/users/erik>*
*!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!*
…On Fri, Feb 25, 2022 at 4:21 PM Negin Sobhani ***@***.***> wrote:
I have not reviewed this code but I have a quick suggestion about the user
interface.
I've noticed here in the namelist the user can choose either dom_nat_pft
or dom_cft to modify the surface dataset. In the subset_data code which
is really similar, the user can choose dom_pdf that they want to modify
and the code calculates if it means changing dom_nat_pft or dom_cft based
on the value of dom_pft. So for example it automatically modifies dom_cft
if it is above 15.
I was thinking maybe having a more uniform user-interface options would be
a good idea between fsurdat_modifier and subset_data. I was wondering what
@ekluzek <https://github.com/ekluzek> and @slevisconsulting
<https://github.com/slevisconsulting> think about using dompft here
instead of two namelist options of dom_nat_pf and dom_cft.
Again, I am mentioning this here as a suggestion to have a uniform
user-interface options for our codes which has a lot of similarities.
Please let me know if I am missing something.
—
Reply to this email directly, view it on GitHub
<#1615 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/ACYCZQEHDS2HTBAD7DEMX7DU5AFF3ANCNFSM5MZIHHAA>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
On @negin513 point about making the user interface more similar. I was about to make an issue, but then I looked and the difference is that this tool has it as a config file, and most of the contents of that file follow what's on the surface dataset.
Since subset_data is using a simpler user-interface I think it's OK that the user interfaces are different. This one more closely reflects what's on the surface dataset being modified. While subset_data is making a command line option that's easier for the user. So I'm siding with the idea that it's OK they are different now. But, I'm also happy to be convinced back the other way again. So I'm open to still having a discussion on this next week. So @negin513 what do you think should we schedule a meeting to discuss? |
|
I agree with @ekluzek. Overall I think it might be good to have our options relatively similar, but maybe we can worry about them in future. For example. previously subset_data had the option of
I intentionally removed my original comment, because upon further looking into the newer version of the code I've noticed you have made this change from |
Oh, interesting. Would you like to open an issue @negin513 to remind us that the |
Description of changes
This PR addresses Isla Simpson's CSSI example (2b), which requires replacement of the vegetation with crop in a rectangle of North America.
I am adding this capability to the modify_fsurdat tool.
Specific notes
Contributors other than yourself, if any:
@billsacks @ekluzek
CTSM Issues Fixed (include github issue #):
None
Are answers expected to change (and if so in what way)?
No. This is a new feature.
Any User Interface Changes (namelist or namelist defaults changes)?
modify_template.cfg now includes the line
dom_cft = UNSETto allow users to set the cft of their choice to replace the non-crop vegetation.
Testing performed, if any:
I have modified a few fsurdat files to confirm that the code does what I want.