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

overwrite this with the version from CABLE-POP-TRENDY #378

Conversation

JhanSrbinovsky
Copy link
Collaborator

@JhanSrbinovsky JhanSrbinovsky commented Aug 30, 2024

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

  • [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
    - 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/

ENDIF
if (mod(ktau, ktauday) == 1) then
climate%dtemp = met%tk - 273.15
climate%dmoist = sum(real(ssnow%wb(:,:))*veg%froot(:,:), 2)
Copy link
Collaborator

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.

Copy link
Collaborator Author

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?

Copy link
Collaborator

@har917 har917 Aug 30, 2024

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.

Copy link
Collaborator

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.

Copy link
Collaborator

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
Copy link
Collaborator

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
Copy link
Collaborator

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 )
Copy link
Collaborator

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
Copy link
Collaborator

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

Copy link
Collaborator

@har917 har917 left a 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:

  1. Care will be needed around the #ifdefs
  2. I suspect that this will need partner updates to cable_def_types_mod() to work.
  3. 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)?

@JhanSrbinovsky
Copy link
Collaborator Author

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,

  • work needs to be done for this cable_climate to even be substituted to offline CABLE, including for applications where it is not called.
  • further thinking into what role this would play in an online CABLE4 is required.

Close this PR until these issues are resolved

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

Successfully merging this pull request may close these issues.

CABLE-POP_TRENDY refactor: core/biogeophys/cable_climate.F90
2 participants