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

mv coupled only file out of science/ #574

Open
JhanSrbinovsky opened this issue Mar 24, 2025 · 12 comments
Open

mv coupled only file out of science/ #574

JhanSrbinovsky opened this issue Mar 24, 2025 · 12 comments
Assignees

Comments

@JhanSrbinovsky
Copy link
Collaborator

Reproducing UM7 PR here first

@JhanSrbinovsky JhanSrbinovsky self-assigned this Mar 24, 2025
@JhanSrbinovsky
Copy link
Collaborator Author

JhanSrbinovsky commented Mar 24, 2025

The LAI clobbering/limiting subr is only valid in coupled apps.

I moved this in UM7 to out of science/canopy and into coupled/shared.

@ccarouge asked me to do this offline CABLE repo. Imagine my surprise to find that it doesnt exist, It is not in science/canopy. searching for the subroutine name (as the file name could conceivably be different) there is:

coupled/JAC/control/shared/LAI_canopy_height_cbl.F90
coupled/AM3/control/cable/shared/LAI_canopy_height_cbl.F90

its a mystery to me how this has got into UM7 and from where.

Before I continue with reconciling/moving all three instances to coupled/shared could @bschroeter please shed some light on this.

@ccarouge
Copy link
Member

@JhanSrbinovsky Looking at the history of the file on GitHub, it looks like it appeared from November. There is only one commit that touches that file but I haven't looked at the commit details (and yes it looks like I'm called Ben 😄 )

@ccarouge
Copy link
Member

This file was removed from CABLE/src/canopy at this commit 8e720127bfb6e0de11074287a4ff303f1354f089.

The idea to do the work from this repository is that we need to do the changes for ESM1.6, AM3 and JAC. So it needs to come from the CABLE repo even if the file is (correctly) not an offline file.

@JhanSrbinovsky
Copy link
Collaborator Author

OK - so it looks like I added it into the UM7 version of canopy/, but when. the shuffle was applied - I thought it overwrote the CABLE that was in UM7 with a subset of CABLE from the offline CABLE repo - Ben @ccarouge ?

coupled/shared files (like this one) are going to exist so it makes sense to keep them in CABLE main, I dont have a problem with that - the one that is here now, post shuffle - where did that come from?

@bschroeter
Copy link
Contributor

Glad to see I don't actually have to respond and can provide useful insight.

Let me know if I can help in any way.

@JhanSrbinovsky
Copy link
Collaborator Author

So where did the file /UM7/umbase_hg3/src/atmosphere/CABLE/src/science/canopy/cbl_LAI_canopy_height.F90 come from then?

@ccarouge
Copy link
Member

ccarouge commented Mar 26, 2025

@JhanSrbinovsky because the repos are not in sync all the time (or never since there is a lot of activity?) we didn't copy from CABLE:main into UM7. We simply moved files in a common organisation in each repo. This ensure the library shuffle only had files that were moved around without any changes in content in the files themselves.

So yeah, no sync has been done in file contents or if a file is only in the science in UM7, it will stay there until the sync work (that you are currently doing) happens.

Edit: this also ensures the pre-processor flags were not messed up with.

@ccarouge
Copy link
Member

We've only copied files that weren't yet in UM7 (like CABLE/src/shared) because they are new files in UM7 so no chance of erasing something and won't have any effect on the code.

@JhanSrbinovsky
Copy link
Collaborator Author

OK Thanks. I think I get it now.

@JhanSrbinovsky
Copy link
Collaborator Author

JhanSrbinovsky commented Mar 28, 2025

As noted in the original description this file is only necessary coupled.

In CABLE:main we have:

coupled/JAC/control/shared/LAI_canopy_height_cbl.F90
coupled/AM3/control/cable/shared/LAI_canopy_height_cbl.F90

In ESM1.6, or UM7, we also have:

src/atmosphere/CABLE/src/science/canopy/cbl_LAI_canopy_height.F90

my intention is to use the same LAI_canopy_height_cbl.F90 across all 3 apps and relocate it to coupled/shared/

Neglecting what is in JAC.

Reconciling the AM3 and UM7 versions. Modifyied UM7 version.

Using this in AM3 and comparing 1 month restarts for bitwise reproducibility yields massive differences. HOWEVER, this is because in the AM3 version, the lower limit to LAI is expressed as 0.99*LAI_thresh. I recall doing this as at one stage a problematic point was sitting on LAI == LAI_thresh. Like many instances this was not the underlying problem however the 0.99 factor remained.

The 1% change on the lower limit to LAI (AND a few extra pixels on the globe were considered vegetated) resulted in completely different 'weather' 30 days later. The demonstrable chaos never ceases to a maze me.

Modifying the largely UM7 suggested/modified version of LAI_canopy_height_cbl.F90 and re-including the factor of 0.99 - and again comparing 1 month restarts for bitwise reproducibility - they're identical.

Next we have to do the ESM1.6 test. Merge both. And to CABLE:main

@har917
Copy link
Collaborator

har917 commented Mar 30, 2025

@JhanSrbinovsky @ccarouge @bschroeter - this issue touches on a much bigger topic. Science-wise this routine needs to be linked into cable_roughness (including dust work in coupled), the connection between the lower limits on canopy height, LAI and roughness (so the various thresholds), #173 - and the general technical refactorisation.

Reconciliation across the repos is a good first step/aim - but does risk raising a lots of questions around what's the correct code and unintentional impacts of changes. What is in the code (all repos) currently is definitely incorrect - but which of the various options is least incorrect I can't say.

@JhanSrbinovsky
Copy link
Collaborator Author

@har917 - I've added this as an issue to revisit. The longish AM3 run I have going should reveal (unintentional) changes due to removing the .99 factor (introduced during debugging in the infancy of AM3). Note however that this .99 was applied to hand-woven, guessed arbitrary thresholds. Technically being able to apply the "solution" across ALL apps can be overcome. The "solution" is more of an obstacle. The version of the LAI/HGT limiting subroutine is the same (formatting aside) as that in ESM1.6 (and also JAC [even more formatting differences]). The only real difference from the reconciled version and the AM3 version was that .99 factor.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

When branches are created from issues, their pull requests are automatically linked.

4 participants