Replace cable_gw_hydro.F90 with its new version #231
Conversation
|
@rkutteh I have seen this but I think it is going to take some time. I believe I have to test it against the main version because there are things in there that can change the results without the groundwater on. |
|
Hi Claire,
no worries, take your time and let me know if you want more info about some of the changes I have made.
We can discuss more on Thursday.
Thanks.
Ramzi
…________________________________
From: Claire Carouge ***@***.***>
Sent: Tuesday, 9 April 2024 12:51 PM
To: CABLE-LSM/CABLE ***@***.***>
Cc: Ramzi Kutteh ***@***.***>; Mention ***@***.***>
Subject: Re: [CABLE-LSM/CABLE] Replace cable_gw_hydro.F90 with its new version (PR #231)
@rkutteh<https://github.com/rkutteh> I have seen this but I think it is going to take some time. I believe I have to test it against the main version because there are things in there that can change the results without the groundwater on.
—
Reply to this email directly, view it on GitHub<#231 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AXR2B4GODFGOTRW3JOCCRETY4NJUTAVCNFSM6AAAAABF3H5DZSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDANBUGA3DANZRGE>.
You are receiving this because you were mentioned.
|
ccarouge
added
the
priority:high
There was a problem hiding this comment.
I have finally finished reviewing this. I have a few minor comments, mainly about the documentation and comments as these won't change the results.
There could be a lot more to change but I know there will be some cleanup done once everything is in and we can check the results. And this is "legacy" code we bring in so we can't be too strict.
| @@ -24,10 +28,12 @@ SUBROUTINE snow_processes_soil_thermal(dels,ssnow,soil,veg,canopy,met,bal,snowml | |||
| TYPE(veg_parameter_type), INTENT(INOUT) :: veg | |||
| TYPE(met_type), INTENT(INOUT) :: met ! all met forcing | |||
| TYPE (balances_type), INTENT(INOUT) :: bal | |||
| REAL, DIMENSION(:), INTENT(INOUT) :: snowmlt | |||
|
|
|||
| REAL, DIMENSION(mp) :: snowmlt !track snow melt | |||
There was a problem hiding this comment.
An annoying requirement from the JULES/UM coding standard:
| REAL, DIMENSION(mp) :: snowmlt !track snow melt | |
| REAL. :: snowmlt(mp) !track snow melt |
I know there are plenty of these to clean up but may as well starting by not adding new ones :)
There was a problem hiding this comment.
@ccarouge A couple of remarks regarding the requested change:
- Is the dot in " REAL. " a typo or is it some new syntax I am not aware of ?
- I realize that the requirement of JULES/UM is a must, but I think it is worth noting that this is not considered good coding practice, as attaching the dimension to the array name is the "old" way of declaring arrays and including the "DIMENSION" attribute in the declaration is the recommended "new" way. I think it is a good idea to double check that this is really what they want so we don't have a repetition of what happened earlier regarding breaking up the modules into smaller ones.
I am happy to commit the suggestion once I hear back regarding the mysterious "dot"
There was a problem hiding this comment.
The dot was indeed a typo. But on reflection, I would leave it as is.
Co-authored-by: Claire Carouge <[email protected]>
Co-authored-by: Claire Carouge <[email protected]>
Co-authored-by: Claire Carouge <[email protected]>
Co-authored-by: Claire Carouge <[email protected]>
|
@ccarouge I have resolved the 4 conflicts. I am ready to merge when permitted, so I can then proceed with working on the next pull request. |
CABLE ground water hydrology work of Mengyuan Mu
This pull request is part of integrating the ground water hydrology work of Mengyuan Mu into CABLE, which will take place in a sequence of pull requests. The actual integration project itself was completed back in September 2023. The current work involves just a gradual merge into the main branch.
Fixes #230
Description
📚 Documentation preview 📚: https://cable--231.org.readthedocs.build/en/231/