-
Notifications
You must be signed in to change notification settings - Fork 6
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
overwrite this with the version from CABLE-POP-TRENDY #378
overwrite this with the version from CABLE-POP-TRENDY #378
Conversation
ENDIF | ||
if (mod(ktau, ktauday) == 1) then | ||
climate%dtemp = met%tk - 273.15 | ||
climate%dmoist = sum(real(ssnow%wb(:,:))*veg%froot(:,:), 2) |
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 think we need to retain a comment here around the change in science from %dmoist = %fwsoil
to %dmoist = SUM(%wb * %froot, 2)
. These quantities are related but different.
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.
such as?
OK before we get too involved, I am. totally unfamiliar with this module - who uses cable_climate?
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.
As far as I know only CABLE-POP simulations actively use this routine - so that's the TRENDY and BIOS applications - however the climate%
is woven throughout the science code.
I'm not actually sure how CM2 and AM3 are handling this to be honest - the TYPE is there (if incomplete) and there is an allocation/initialisation. However there is no call to the cable_climate()
subroutine yet there is a use (in AM3) in casa_rplant
and cbl_dryLeaf
- I would have thought that would have caused some 'interesting' results or a crash.
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.
To answer the implicit question - all we need to do is include a comment such as
! Has been updated from %dmoist = canopy%fwsoil
with, if possible, a svn revision number.
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.
there is a use (in AM3) in casa_rplant and
cbl_dryLeaf
Specifically casa_rplant()
and cbl_dryLeaf
use climate%qtemp_max_last_year
- this is initialised to 0.0. HOwever both uses are inside a IF cable_user%CALL_climate
condition so will only be active if triggered by the user.
|
||
!jhan: see CABLE Ticket#149 and above CABLE_LSM: comment |
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.
May need to retain this #ifndef
condition - I don't know why it was necessary and likely superceded by the #ifdef UM_CBL
|
||
!jhan: see CABLE Ticket#149 and above CABLE_LSM: comment | ||
# ifndef ESM15 |
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.
see earlier comment about this #ifndef
|
||
! ============================================================================== | ||
|
||
SUBROUTINE WRITE_CLIMATE_RESTART_NC ( climate, ktauday ) |
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.
Where have the write_climate_restart_nc
and read_climate_restart_nc
subroutines/capability gone? Ah-ha possibly into write_netcdf_cbm_var()
which is in cable_def_types_mod
.
So this PR may need to deal with that MODULE as well.
|
||
! Aug 2017: additional leaf-level met variables stored for use in optimising ratio | ||
! of Jmax to Vcmax | ||
#ifndef UM_CBL |
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.
mmm - not sure I really like this in the code base at least not without the corresponding #ifdef case
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 think this is okay to go in with some key notes:
- Care will be needed around the
#ifdef
s - I suspect that this will need partner updates to cable_def_types_mod() to work.
- this is one of the routines that will need to be rationalised as part of CABLE-4 - i.e. this PR should be viewed only as a temporary fix.
@ccarouge in light of the above - are you happy that @JhanSrbinovsky goes ahead and merges into MAIN (assuming that any technical quirks can be overcome)?
the ifndef um_cbl was because it is NEVER used in any UM application and there were corresponding things to bring in at build time which would have complicated things for a subroutine that isnt even called. That said - the version from CABLE-POP is even worse. They've added things to climate% TYPE which is I suppose expected. They've even added elements to met% TYPE. Online, climate% TYPE is declared etc, they are just never used in calculation of anything (well they shouldn't be anyway, I hope this is the case). There will be more work needed to even get this to build properly! I would be tempted to just abandon this altogether, except that it needs to go in if indeed there is to be a CABLE4 code base. At the very least there will have to be things "taken out" (or reviewed if they are even necessary) that apply to reading and writing netcdf files. In summary,
Close this PR until these issues are resolved |
CABLE
Thank you for submitting a pull request to the CABLE Project.
Description
This keeps getting in the way of merging AND I dont think anyone else uses this SUBROUTINE? Hence requesting review by Ian
Fixes #251
Type of change
Checklist
- ALTHOUGH this is as it appears in CABLE-POP-TRENDY.
- Tidying and ensuring compliance at this stage would make the move redundant
📚 Documentation preview 📚: https://cable--378.org.readthedocs.build/en/378/