Skip to content
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

Improve handling of use_lai_streams for PLUMBER2 sites #2185

Open
ekluzek opened this issue Oct 6, 2023 · 5 comments
Open

Improve handling of use_lai_streams for PLUMBER2 sites #2185

ekluzek opened this issue Oct 6, 2023 · 5 comments
Assignees
Labels
code health improving internal code structure to make easier to maintain (sustainability) enhancement new capability or improved behavior of existing capability

Comments

@ekluzek
Copy link
Collaborator

ekluzek commented Oct 6, 2023

In #2155 we had to do some hacks to handle use_lai_streams where the shell_commands appends to the user_nl_clm file. The same is true for history output in BGC (default) vs. SP cases.

This is problematic, because it will keep appending every time shell_commands is run, ordering might be important, and it's problematic to clone. More robust would be to work inside of build-namelist and/or buildnml to do this by reacting to CLM_USRDAT_NAME settings for PLUMBER2.

@ekluzek ekluzek added enhancement new capability or improved behavior of existing capability code health improving internal code structure to make easier to maintain (sustainability) labels Oct 6, 2023
@ekluzek ekluzek self-assigned this Oct 6, 2023
@wwieder
Copy link
Contributor

wwieder commented Oct 6, 2023

Thanks for adding this, Erik. Is the build-namelist smart enough to know if it's using an SP vs. BGC compset?

Generally, I wouldn't advise cloning between these two types of runs anyway, so a case that fails (e.g., because someone is trying to run BGC case from an SP clone) seems OK?

@ekluzek
Copy link
Collaborator Author

ekluzek commented Oct 6, 2023

Yes, build-namelist can distinguish the type of compset.

You are right that if a user does a bunch of SP or BGC specific changes to a case, cloning that to the other type is not going to go well. So it's fine to discourage that.

The concern I see is about doing any kind of clone. There is a strange relationship now between the user_nl_clm file and shell_commands, so that causes some strange side effects in the case itself, and I think it might with clones as well. In principle this is the kind of hacky thing that I think will potentially cause unforeseen issues, so we should be nervous about it. One of the side effects is that the user_nl_clm will contain a bunch of duplicate lines, that will be confusing to users, especially new users. That's not really that bad. But, I'm still nervous this will do something weird for clones or some other case we haven't thought of.

In the spirit of being agile (as in Agile Software Development Methodology), we are bringing in the simple implementation with the plan to refactor it to improve it. This gets working code quicker in quicker, but allows us to improve it later. I also get nervous about working that way as we often bring things in and then don't have time for the needed cleanup afterwards. And not doing the needed refactoring means that it makes future developments harder to do, and makes debugging harder for future users. So it's a short term gain for longer term impacts. There's also times when an initial simple implementation might be sufficient. Being agile we decide what refactoring is important to do to prevent those long term impacts, and allow simple implementation to exist when we don't think it's worth improving.

@wwieder
Copy link
Contributor

wwieder commented Oct 6, 2023

This makes sense. I also wondered about the likelihood of addressing this down the road? Not because I'm arguing it should be done now, but if this issue would eventually get a won't fix designation?

@wwieder
Copy link
Contributor

wwieder commented Apr 1, 2024

FWIW, history variables would also be nice to clean up here too.
for an example of how messy this currently looks see /glade/u/home/wwieder/scratch/PLUMBER2_tests/US-NR1_SP_test/user_nl_clm

@wwieder
Copy link
Contributor

wwieder commented Apr 1, 2024

In the refactor of run_tower.py I was going to ask if we could easily add a flag for BGC or SP cases. I would also be nice to extend this to FATES-SP and FATES-noComp configurations. Handling all of this is usermods seems like a nightmare.

I don't want to keep having scope creep into work @TeaganKing is doing for the PLUMBER2 work, but I also want to avoid reinventing the wheel every time we want to expand the functionality of these single point capabilities. What level of support do we want to provide for supported tower simulations? At what point do we encourage users to set up their own cases manually?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code health improving internal code structure to make easier to maintain (sustainability) enhancement new capability or improved behavior of existing capability
Projects
None yet
Development

No branches or pull requests

2 participants