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

Cleanup of Catchment-CN constants in CatchmentCNRst.F90 #682

Merged
merged 31 commits into from
Mar 7, 2023

Conversation

gmao-jkolassa
Copy link
Contributor

@gmao-jkolassa gmao-jkolassa commented Dec 8, 2022

This PR addresses parts of issue #667 . It removes the duplication of Catchment-CN specific constants in CatchmentCNRst.F90 and instead imports them through use-statements.

I tested this branch in a 2-month experiment for Catchment-CN4.0 and Catchment-CN4.5 and verified that restart files generated by ldas_setup and written out at the end of the 2-month period were identical to the equivalent restart files from experiments run with the 'develop' tag.

TO DO: So far, this branch only fixes the issue of the duplicated Catchment-CN constants, but does not address the duplication of orbital parameters (and the use of outdated orbital routines pointed out by Peter Norris). @weiyuan-jiang , do you want me to also make the changes for the orbital parameters or would you prefer to do that yourself?

cc: @gmao-rreichle

Update 03/03/2023:

This PR has been expanded and now includes the following.
It removes the duplication of Catchment-CN specific constants in CatchmentCNRst.F90 and instead imports them through use-statements. It also removes the duplication of orbital constants and replaces the use of hard-coded orbital subroutines with the relevant MAPL subroutines.

This PR should be zero-diff for Catchment and is zero-diff for Catchment-CN when used with RESTART: 1 (restarts are of the same resolution as model run). It is not zero-diff for Catchment-CN used with RESTART: 2 (restarts are of a different resolution than model run). When running Catchment-CN with RESTART: 2, there are differences in the day length variable (dayx) with maximum differences on the order of 10 seconds.

Because changes had to be made to MAPL, this PR is dependent on using MAPL tag 2.35.0

@gmao-jkolassa gmao-jkolassa added 0 diff The changes in this pull request have verified to be zero-diff with the target branch. cleanup labels Dec 8, 2022
@gmao-rreichle gmao-rreichle linked an issue Dec 8, 2022 that may be closed by this pull request
@gmao-rreichle
Copy link
Contributor

Thanks, @gmao-jkolassa, for this first part. Re. the orbital parameters: It's probably better if you take a first crack at it, assuming that Peter talked with you about what needs to be done (in a nutshell: use the day-length calculation from MAPL, rather than doing a custom calculation). Does that make sense?

@gmao-jkolassa
Copy link
Contributor Author

Thanks, @gmao-jkolassa, for this first part. Re. the orbital parameters: It's probably better if you take a first crack at it, assuming that Peter talked with you about what needs to be done (in a nutshell: use the day-length calculation from MAPL, rather than doing a custom calculation). Does that make sense?

Sure, I'll get started on that and will check back with Peter if anything is unclear.

@weiyuan-jiang
Copy link
Contributor

The CI-build fails now because it needs develop branch of MAPL.

@gmao-jkolassa
Copy link
Contributor Author

Just wanted to comment that I have been using the MAPL develop branch ever since Peter's changes got pushed to it. I was updating to MAPL develop after cloning the repo. The model has successfully built since updating, but I do see differences in some variables (as expected) and need to establish that these differences are within reason.

@gmao-jkolassa
Copy link
Contributor Author

In the prior version there was still a bug where the latitude information was passed to MAPL_SunGetDaylightDuration() in degrees instead of radians, which led to an unreasonable geographic distribution of day length values. My latest commit fixes this issue. Compared to the implementation in develop, the maximum differences in the day length (dayx) are now on the order of 10s, which is within the expected range. All other variables are zero-diff. This is the case for CatchCNCLM40 and CatchCNCLM45. So from my end, this is now good to go.

Note: As of right now, this branch is up to date with the current develop branch. However, it does have to be used with the current develop branch of MAPL.

@gmao-rreichle gmao-rreichle added the Non 0-diff The changes in this pull request are non-zero-diff label Mar 7, 2023
Copy link
Contributor

@gmao-rreichle gmao-rreichle left a comment

Choose a reason for hiding this comment

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

Approving for Land Team and Surface Preproc Team.

@gmao-rreichle gmao-rreichle marked this pull request as ready for review March 7, 2023 17:17
@gmao-rreichle gmao-rreichle requested review from a team as code owners March 7, 2023 17:17
Copy link
Member

@mathomp4 mathomp4 left a comment

Choose a reason for hiding this comment

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

Approve for CMake

@sdrabenh sdrabenh merged commit d4775d5 into develop Mar 7, 2023
@sdrabenh sdrabenh deleted the feature/jkolassa_catchCNrst_cleanup branch March 7, 2023 19:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
0 diff The changes in this pull request have verified to be zero-diff with the target branch. cleanup Non 0-diff The changes in this pull request are non-zero-diff
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Avoid duplication of constants definition in CatchmentCNRst.F90
6 participants