Dev merge eclm with FruitTree and WinterCrops all at once #95
Conversation
This reverts commit 43529b6: Revert the change in urbantv for clm4_5 as it changes answers in some cases.
…st appears to be not necessary and second screws up the cold start
… seperate part of driver can be called from. Also add some SHR_ASSERT calls for checking it, add a new diagonstic history variable for H2OSOI as read from streams so can compare to input, make the calls to new subroutine in the original place which shouldn't change answers, but it does unexpectedly
Properly send $opts down to it, and use it in calls inside. Also add a check that this isn't a transient use case for soil streams with linear time-interpolation as that will likely cause an error going from a vegetated point that is missing to not, or vice versa. Also add a check for that. And fix the soilm test which had phys in place of conopts.
Change h2osoi_prs to be on gridcell. Do some checking for spval in soil moisture streams. Added a check that prints a warning and a commented out endrun (until we have a dataset in full agreement).
Add new namelist item soilm_ignore_data_if_missing and react to it. Also use old way for the indexing in an attempt to reproduce the previous results. If soilm_ignore_data_if_missing is true and find a missing point expected continue calculation as before. If not abort when that point is found. Also comment out the section where values greater than 10 were set to spval as that changes answers.
OK, this is a version that is identical to release-clm5.0.27. Found out the issue was that lai_interp can't be moved the way I did. It needs to be divided into the streams part and non-streams part like is being done in SoilMoistureStreams. So this replicates answers for SMS_P720x1_D_Ld3.f09_g17.I2000Clm50SpGs.cheyenne_intel.clm-prescribed
… level and SoilMoistureInterp to the OMP clump level
…nd lai_interp called from within an OpenMP threading loop
Add tintalgo settings for most of the streams. Also change the buildnml fatal-error for soilm_tintalgo='linear' for transient to a warning that can be overridden if desired. Also fix the build-namelist unit test for warnings, so it verifies that warnings die, but will run if -ignore_warnings is set. Also get maxpatch_pft to normally die if the two valid settings are used (79 or 17).
…or soil moisture streams)
…nes. First appears to be not necessary and second screws up the cold start" This reverts commit ac16068.
…me that gets them to work)
Error message example: ``` /bin/sh: 1: ./configure: Permission denied ```
…9) Merge commit 'f06da89b136147493ee0d555429ca888621a473c' into eclm
…Nr 33), Merge commit 'c434963afc5bee1b34fbfdd4ac561a070198da29' into eclm
…66ffb5e4711a68b02236' into eclm
…commit '700e5c761b67029615c74c6ed045ce4203367ef6' into eclm
…52465bd67f1392cd5c8adf97fa91a803c405' into eclm
…ith_FT_WC_all-at-once Merge remote-tracking branch 'https://github.com/HPSCTerrSys/clm5_0/tree/eclm' into 'https://github.com/HPSCTerrSys/eclm/tree/dev-merge-eclm_with_FT_WC_all-at-once' This will include: - FruitTree developments - Winter Crops developments
AGonzalezNicolas
requested review from
DCaviedesV,
jjokella,
kvrigor,
mvhulten and
s-poll
|
Thanks for starting this! I have two questions
|
|
I don't think this big merge is a good idea... those TODO list should be their own individual PRs (like #45) which is small enough for a proper review |
There is a reasoning for this. The merge between the two branches and CLM5.0 and eCLM has already been done. This PR is about bringing this code up to date with eCLM. I understand the concerns both from @jjokella and @kvrigor about the size and scope of the PR. However, re-doing the work that was done already in resolving the divergences introduced by the branches and the CLM5/eCLM development. In my opinion, this is an exceptional case, and thus has to be handled differently than the workflow we would like to enforce. |
|
I still think a big PR like this is basically unreviewable, and I find it unsettling to rely on a merge-and-pray approach. Some specific issues:
I think conflict resolution only works when code is reviewable; hence the common practice of small, reviewable PRs. Also, |
|
Hm, in the end all the history will anyway go away again, once the PR is squashed, right? So it may be cleaner to just copy the files and have a single commit... More important in my opinion is to have a pure Fruit-Tree PR and not to mix it with the still under review #45. My last inspection of the #45 gave me the impression that it should itself be chopped into sub-PRs that can be checked for mergeability (tests + code review that ensure backward-compatibility). I guess, it is best to discuss this tomorrow in our meeting. |
I think that PR should be fine; the changes represent what Theresa did to support WinterWheat--no more, no less. Besides, we want to verify Theresa's change all at once, not smaller parts of her work. Also, if FruitTree would cause some problem in the future, we would want to revert a single commit, not a series of commits with legitimate commits mixed in between |
|
I don't disagree with @kvrigor points, in general. I think, however, we need to carefully evaluate the alternative and structure a pragmatic approach. Some specific comments:
I don't think this is the case. The testing has been done in the integration, and a second set of testing merging the devs with the current head of eCLM should be robust. Not really merge-and-pray, but I do agree that it is convoluted.
In principle yes. But it is way to late in the development process of these features to engage in this. This is a failure of process. We now have to solve it.
Fully agree. But also a failure of process. At this point in this development, this is already the spilled milk.
This is the crux of the matter in my opinion. This is the testing that needs to be done. However, as far as I understand, the testing of the FruitTree + WinterWheat merge into CLM5/eCLM has already been carried out. So we have a baseline to answer this. @AGonzalezNicolas can weigh in with specifics.
Sure, but this is true always. I understand a big PR obfuscates this further. But assume that WinterWheat and FruitTrees had been developed as a single development. Then this simply means there is a big PR. It is not nice, but does it pay off to undo it?
Fair. Nothing against this.
I also agree with this. However, my point is that the divergences already exist. This is a fact, and therefore we have to resolve them pragmatically. Since the work of re-converging these divergences is already advances in the branch in this PR, it seems counterintuitive and counterproductive to start again from scratch. The granularity would be, at best, to split WinterCrops from FruitTrees and then have two PRs, and in each of them re-do what is already done here. I would cast two questions: |
|
Hi all, I also agree with basically everything and think it is a matter of finding the most pragmatic and robust way forward. Still insisting that I am a friend of splitting PRs up. Very concrete point: There are Another quick cleanup: Some permission change like After this, all changes of this PR should be restricted to |
|
@DCaviedesV, @jjokella, @kvrigor I updated the pull request comment with more details describing the procedure I followed. Let me know if anything is unclear or if further information would be helpful. |
|
My biggest qualm is when in the future somebody observes a "weird model behavior" of eCLM. With this big PR there is no way to revert into an old commit that corresponds to a single feature. This makes future investigation of weird behaviors extremely difficult. Another big problem is this PR carries the old CLM5 git history which should have been a separate PR in itself. Perhaps it should be a GitHub issue first; to be honest I'm still not fully convinced of its value. Users care about model features that is relevant to their simulation, not the particulars of our git repository. And some comments:
Fully agree; this is the most pragmatic approach. Small PRs are reviewable, limited scope means test results are meaningful, and the review process is smoother. Safer, reviewable, testable, faster.
I struggle to see the problem in the process: this is the 95th PR and the ones before that went well, i.e., well-scoped and reviewable PRs. The recent eCLM-PDAF PRs of Johannes and Yorck perfectly demonstrate this process.
I actually do not like this 😅 I trust people in our group could write issues on their own without relying on a template, and so far it works. The last thing I wanna read is a GitHub issue that reads like a travel visa application form...
This PR has a huge upfront cost in terms of effort and review time. Our usual approach might be slower, but it is steady and takes much less effort per iteration. Every PR is an opportunity not only to catch bugs, but to collectively learn about the codebase. I fail to see a strong argument for combining old CLM5 history+FruitTree+WinterWheat into a single PR. Too much code to review, too many results to analyze, too many steps done when much fewer could have been made. In my opinion, effort could have been much better spent in iterative changes in the form of small PRs. |
|
To add: since PRs get squashed as a single merge commit into master, preserving the CLM5 history in this branch would have no use. |
|
We had some interesting discussion in the SDLTS meeting on 17.12.2025, which included thinking over alternative in which to do this which go in the direction of minimising effort and risk. The first outcome of the discussion is @AGonzalezNicolas edited the PR description with a more detailed recollection of what was done to generate this. This is relevant, and links to @kvrigor's comment
I'm not sure whether we should squash the commits or not. The original reasoning and requirements for the merges of the WC and FT branches was we could propagate them both into eCLM and into the upstream CLM5. That motivated this idea of recovering the history between eCLM and CLM5. This of course relates to @kvrigor's comment :
At some point this was agreed to be a valuable strategy. I agree this is mostly not useful for users. But useful for us to be able to reconcile eCLM-CLM5 divergences (for example to uptake new CLM5 devs from upstream, or to push eCLM devs upstream to CLM5). With this in mind, we discussed in the meeting that it could perhaps make sense to separate the different steps that @AGonzalezNicolas did (relying on intermediate states in the history of that dev) and cast them into several PRs, the first one concerning the CLM5 git history, in the spirit of
Concering this:
The failure of the process starts many years ago when the original dev WC and FT branches started and multiple divergences occurred. It is not a failure of the current established process (with well engineered, small scoped PRs). This discussion is about recovering from a not-well-managed RSE process from long ago (aka spilled milk).
Yes, but a large part of that cost has been paid already. That is precisely my point. What I would like to know is the concrete alternative which satisfies all the original requirements (i.e., including recovering the CLM5 history), which does not imply paying this cost again.
Possibly yes. This is along the lines of what we discussed yesterday in the SDLTS meeting (see the beginning of my comment). But I would oppose starting the process from scratch, when so much has been done. I think we need to consider if we can use the work @AGonzalezNicolas already put in, which was quite a lot, to satisfy all the original requirements. |
From the POV of a domain expert whose job is to get things done, I don't see this as a failure. We're already lucky that Theresa and Olga encapsulated their work in a form of git repo. I believe it's part of our job to straighten up the code from domain experts (who have way better things to do than deal with code divergence) and make sure it cleanly gets into master. Again, the recent eCLM-PDAF PRs by Johannes/Yorck/Fernan already demonstrated this. We should be helping the feature authors to faithfully merge their work onto master, not spoil it.
I think we're trying to hit three birds at once here: add FruitTree, WinterWheat, and CLM5 history. The more relevant question here is, why bundle these three separate features in the first place? Justifying it through effort that has already done before is not really a reason.
See #45. It could have been already merged if the test effort went toward that PR. And notice how brief that PR is—even the patching strategy is in a form of a shell script which anybody can easily replicate. If it helps, I could easily create a similar PR for FruitTree. The only bottlenecks in this kinds of PR is manual verification.
My disappointment is there is so much wrong thing has been done that no amount of lengthy description + promise of comprehensive tests will save this PR. The most natural and easiest thing to do is one PR at a time: #45 is already there, a FruitTree PR can be easily created, and the CLM5 history discussion on a separate GH issue. There are so many other points I'd like to discuss, but I'll stop here to not spawn new discussion threads 😅. This PR has spawned multiple points of debate which reflects the complexity of this PR. Because of that we weren't able to focus our discusion in the most important part of a PR—the features themselves! |
PULL REQUEST info
This pull request wants to include the following developments:
Into the last eclm-commit a72229c of repo HPSCTerrSys/eCLM/ by merging branch-eclm of repo HPSCTerrSys/clm5_0. Below there is a detailed explanation how branch-eclm of repo HPSCTerrSys/clm5_0 is produced.
Before accepting this pull request, the following needs to be checked:
Next steps should include to run these 3 cases in CI.
This should conclude issues: #94, #93, #45
This only can be done with scientific support from IBG-3 (e.g., @tboas, @Toeroeoe).
STEPS FOLLOWED TO MERGE FT and WC into clm5.0:
Start two new dev-branches with the most updated clm5.0 version at that moment (commit e7285f6caeec6f0b35b25a6d49cb59f41d1a0c3e) in our HPSCTerrSys/clm5 repo
Merge WC (
tags/reelase_08_2020) development with one of the new dev-branches -->release-clm5.0_WinterCropsREADMEfile.Merge FT with the other new dev-branch -->
release-clm5.0_FruitTreeREADMEfile.Replace variable names on FT branch -->
release-clm5.0_FruitTree_namesVarrelease-clm5.0_WinterCropswas different fromrelease-clm5.0_FruitTree.release-clm5.0_WinterCropsused variables ascovercrop_1andcovercrop_2.release-clm5.0_FruitTree:These variable replacements are found in this other branch release-clm5.0_FruitTree_namesVar.
release-clm5.0_FTWCI used
Visual studio codeto manually solve conflicts one by one (no agents, no AI involved).The branch with both merged developments is clm5_FTWC.
Case files here
Case files here
Once the merge is considered successful (after validation against the reference cases), I proceeded to change structure of clm5 to eclm while preserving the commit history of clm5.
STEPS FOLLOWED from clm5-structure to eclm-structure
release-clm5.0_FTWC--> eclm_base_00bootstrap_v2_FTWC_00--> very similar to what @kvrigor had applied to fork eclmAnd solve conflicts on
Viscual Code Studiomanually.