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

402 merge from am3 params #404

Merged
merged 15 commits into from
Nov 6, 2024
Merged

402 merge from am3 params #404

merged 15 commits into from
Nov 6, 2024

Conversation

JhanSrbinovsky
Copy link
Collaborator

@JhanSrbinovsky JhanSrbinovsky commented Sep 17, 2024

CABLE

Thank you for submitting a pull request to the CABLE Project.

Description

Merge modifications made during in AM3 development.

You can link issues by using a supported keyword in the pull request's description or in a commit message:

Fixes #402

Type of change

Merge modifications made during in AM3 development.

Testing:

benchcab regression testing against main show no changes.

Checklist

  • The new content is accessible and located in the appropriate section.
  • I have checked that links are valid and point to the intended content.
  • I have checked my code/text and corrected any misspellings

📚 Documentation preview 📚: https://cable--404.org.readthedocs.build/en/404/

@JhanSrbinovsky JhanSrbinovsky linked an issue Sep 17, 2024 that may be closed by this pull request
Copy link
Member

@ccarouge ccarouge left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the delay. I forgot to final step of the review process.

src/params/grid_constants_cbl.F90 Outdated Show resolved Hide resolved
src/params/grid_constants_cbl.F90 Outdated Show resolved Hide resolved
@ccarouge
Copy link
Member

The regression testing should happen in benchcab. The user guide is here: https://benchcab.readthedocs.io/en/latest/

I would expect the following config file to do what you need:

# Example configuration file for running the CABLE benchmarking.
# 
# Note, optional keys are available in this config file. See
# https://benchcab.readthedocs.io/en/stable/user_guide/config_options/
# for documentation on all the available keys.
# 
# This file is in YAML format. You can get information on the syntax here:
# https://yaml.org/spec/1.2.2/#chapter-2-language-overview
# Quick tips:
# You need a space after the ":"
#
# It uses the same syntax as Python for:
#     - lists (aka sequences)
#     - dictionaries (aka mappings with key/value pairs)
#
# Strings can be given with or without double or single quotes.

realisations:
  - repo:
      git:
        branch: main
  - repo:
     git:
       branch: 402-merge-from-am3-params
  
modules: [
  intel-compiler/2021.1.1,
  netcdf/4.7.4,
  openmpi/4.1.0
]

And you don't need to do the analysis on modelevaluation.org. Just check the log file to ensure the regression testing was all successful.

@JhanSrbinovsky
Copy link
Collaborator Author

JhanSrbinovsky commented Sep 23, 2024

@ccarouge, I can run benchcab and get results from dozens of runs but the documentation isnt clear WRT what the output is. So what is S0_RO, etc. I have two beaches A and B. How can I see that A is effectively identical to B?

@ccarouge
Copy link
Member

@JhanSrbinovsky This part in the documentation should tell you where to look:

runs/fluxsite/analysis/bitwise-comparisons
directory that contains the standard output produced by the bitwise comparison command: benchcab fluxsite-bitwise-cmp. Standard output is only saved when the netcdf files being compared differ from each other

But I agree, we need to make it clearer where people can find the information about the results in the docs.

@JhanSrbinovsky
Copy link
Collaborator Author

Doh - so you're telling me that the only directory that sounded like what I thought might be relevant was empty by design

@ccarouge
Copy link
Member

The PBS log file should also have human-friendly logs of success for the regression tests.

@JhanSrbinovsky
Copy link
Collaborator Author

JhanSrbinovsky commented Sep 24, 2024 via email

@JhanSrbinovsky
Copy link
Collaborator Author

DOH! - I've got myself in a muddle here. I've started to make changes pushing nCNP_pools index to coupled only, but I have done work in a new issue #412 (+ I have a local branch going) PLUS I have had to make the corresponding changes in AM3 to make sure it works there. Between juggling the kids and a new puppy I've got all these branches mixed up.

cable_surface_type_namelist - which will be implemented here as well at
a later date
@JhanSrbinovsky JhanSrbinovsky reopened this Oct 7, 2024
@JhanSrbinovsky JhanSrbinovsky self-assigned this Oct 7, 2024
@JhanSrbinovsky JhanSrbinovsky added the priority:high High priority issues that should be included in the next release. label Oct 7, 2024
@JhanSrbinovsky JhanSrbinovsky added this to the ESM1.6 FastTrack milestone Oct 7, 2024
@JhanSrbinovsky
Copy link
Collaborator Author

BTW - renewed regression testing not only shows no unintended changes in output but absolutely zero changes in output

# CABLE

Thank you for submitting a pull request to the CABLE Project.

## Description

Miscellaneous developments in science/ dir. from AM3. These are included
separately to more major developments in canopy and soilsnow
directories.

Fixes #402

## Type of change

Including AM3 development.

## Checklist

- [X ] The new content is accessible and located in the appropriate
section.
- [ X] I have checked that links are valid and point to the intended
content.
- [ X] I have checked my code/text and corrected any misspellings

<!-- readthedocs-preview cable start -->
----
📚 Documentation preview 📚:
https://cable--430.org.readthedocs.build/en/430/

<!-- readthedocs-preview cable end -->
@JhanSrbinovsky
Copy link
Collaborator Author

@ccarouge - I can't move on this until approved. From what I can tell requested change was made. what else is there?

Copy link
Member

@ccarouge ccarouge left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, can't approve. Some changes can't go in. Especially, changes in the radiation would remove a bug fix. We want to keep this fix in main and implement it in ESM1.6, not the other way around.

src/params/grid_constants_cbl.F90 Outdated Show resolved Hide resolved
src/science/misc/cable_carbon.F90 Outdated Show resolved Hide resolved
src/science/radiation/cbl_init_radiation.F90 Outdated Show resolved Hide resolved
src/science/roughness/roughnessHGT_effLAI_cbl.F90 Outdated Show resolved Hide resolved
@ccarouge ccarouge mentioned this pull request Oct 31, 2024
5 tasks
JhanSrbinovsky and others added 3 commits November 1, 2024 10:09
Co-authored-by: Claire Carouge <ccarouge@users.noreply.github.com>
@JhanSrbinovsky
Copy link
Collaborator Author

@ccarouge - I think I have addressed all the requested changes. It was a few days ago since I last visited this. Can't remember if I re-requested review but that doesnt appear to be an option, suggesting that I have. "see review" doesn't seem to show any others

Copy link
Member

@ccarouge ccarouge left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One more question. Why are there a bunch of files with the line:
!#define UM_CBL YES

I would have thought files in CABLE offline should have this as NO?

@JhanSrbinovsky
Copy link
Collaborator Author

JhanSrbinovsky commented Nov 6, 2024

I am using if UM_CBL is defined or not, not what it is defined as. You can also test what it is defined as (I forget the exact syntax) but the test alone is more complicated than just ifdef. As it is switching b/n coupled and not is only 1 character, "!", which by default is in place, i.e. it is NOT defined. The alternative was going to involve two lines, switching between the two would involve commenting the other one, and all of t his for a variation I was hoping would be gone by now, possibly it will soon-ish anyway :). Maybe not b4 Xmas, but in the 1st half of next year. @ccarouge

Copy link
Member

@ccarouge ccarouge left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Finally! Sorry for the very lengthy delay.

@JhanSrbinovsky JhanSrbinovsky merged commit a5a52d8 into main Nov 6, 2024
7 checks passed
@JhanSrbinovsky JhanSrbinovsky deleted the 402-merge-from-am3-params branch November 6, 2024 05:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority:high High priority issues that should be included in the next release.
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Merge from AM3 - params
2 participants