-
Notifications
You must be signed in to change notification settings - Fork 5
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
consistency_ice_veg_soil #349
Conversation
@ccarouge @JhanSrbinovsky @har917 While attempting to run benchcab on my consistency_ice_veg_soil branch and main I am getting the same errors from both codes:. The error is coming from cable.nml , specifically someone set check%ranges = 0 instead of a logical *from .out: Cannot process command line definition of namelist fie using cable.nml as the namelsit *from .err: forrtl: severe (17): syntax error in NAMELIST input, unit 10, file /g/data/w97/rk4417/projects-svn/cable-dev/iceconsistency_benchcab/runs/payu-laboratory/work/crujra_access_R0_S0/cable.nml, line 26, position 21 |
@rkutteh @ccarouge @abhaasgoyal MAIN is undergoing a transition from The 'easy fix' will be to include
in your benchcab.yaml file. The better solution would be to update your dev branch with MAIN - but that possibly too much of a task for your current work. @ccarouge any other options? |
@har917 main itself fails to run, not just my branch |
@rkutteh Sorry, we broke benchcab with an update to CABLE. This triggered some thoughts on how best to fix but also on how to avoid that in the future. This resulted in a delayed solution. The solution is coming very soon. For you, you'll need to add the following to your benchcab config.yaml:
Then, both realisations should have the correct format for their namelists. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have merged in main into this branch in order to get spatial outputs with benchcab.
Looking at the outputs I couldn't quite say what was happening and saw the information from the consistency checks is missing from the log file so I have one change requested.
Once this is in, we can run benchcab and have a quick check on how the results are affected.
src/offline/cable_parameters.F90
Outdated
WRITE(*,*) 'SUBROUTINE load_parameters:' | ||
WRITE(*,*) 'At land point number ', i | ||
WRITE(*,*) 'And patch number ', k | ||
WRITE(*,*) 'isoilm is ICE_SoilType' | ||
WRITE(*,*) 'Set rhosoil = density_ice from CABLE physical constants' | ||
WRITE(*,*) 'Set css = csice from CABLE physical constants' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The write statements need to be directed to logn
logical unit so that they are written in the CABLE log file and not the standard output.
logn
is in cable_IO_vars_module
which is in the USE statements on this cable_param_module
module so it shouldn't need to be passed in by argument.
This is a comment valid for all the WRITE() statements not just the ones here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ccarouge Happy to make the change but just want to point out before I do so that I modelled these write statements after the ones in SUBROUTINE check_parameter_values. Overall there are 41 occurrences of such WRITE(,) in cable_parameters.F90. I am guessing then that you are planning eventually to change all of them ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I made the requested changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We will work on existing stuff later. For the moment, let's make sure the new stuff gives the info in the correct place. Thanks for making the change.
changed write statements inside consistency_ice_veg_soil subroutine to output to logn instead of standard output
Running the spatial configuration in benchcab, we get: The differences definitely show where we expect them. |
@ccarouge @rkutteh I'm a bit surprised that there's a grid cell in Iceland with dominant ice-iveg which has necessitated the swtich to ice soil ... but I don;t know enough about this spatial configuration to know whether to worry about that. Otherwise it looks plausible .. certainly the changes are all in regions where we would expect them. The question around whether to put under a namelist really comes down to how the community wants to handle this kind of 'noticeable bug-fix' - see the #354/#355 discussion. I don;t think we've settled on a position on this topic yet. |
Interesting. I wouldn't put this as a bug fix. The code is working perfectly well before and after the change, we haven't changed how the model calculates the outputs or reads in the inputs. We add a "clean up" of the input data. I would consider this a new feature, as it allows people to forgo that QA step in their setup (it's irrelevant that it wasn't routinely done) and let CABLE do it for them. Obviously, not having this on can lead to inconsistent inputs resulting in erroneous outputs, but it isn't because CABLE has a bug, it is because proper QA on the inputs hasn't been done. |
@rkutteh I'll feel better with having this with a namelist option because as said earlier this is a new functionality and not a bug fix for me. I'll try and get this namelist option in today. |
Checked when using the namelist option set to true, we got the same results as without the namelist option. And results when set to false are the same as before the change. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's good for me
@rkutteh Can you check what I've done for the namelist option? And what I have added to the docs before we close this off. I can't set you as a reviewer because you've opened the pull request. |
Yes, will do |
Everything looks good. You may want to add the new ice consistency option to cable.nml with its default setting, as a way of informing the user, but that's up to you. Please let me know when you are happy for me to merge (I realize you already approved). |
CABLE
Add a subroutine to CABLE to ensure the consistency of ice points between soil and vegetation, related to #338.
This branch will be used for the benchcab testing.
Fixes #280
Fixes #338
📚 Documentation preview 📚: https://cable--349.org.readthedocs.build/en/349/