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

set defaults for new ED2IN vars #3033

Merged
merged 14 commits into from
Sep 28, 2022
Merged

set defaults for new ED2IN vars #3033

merged 14 commits into from
Sep 28, 2022

Conversation

Aariq
Copy link
Collaborator

@Aariq Aariq commented Sep 13, 2022

Description

Sets some arbitrary defaults for new variables in ED2IN file. These are required for the ED2IN file to be parsed, but I don't think they actually are used. The ED2IN template says "These variables are used to define the additional soil properties needed for SOIL_HYDRO_SCHEME=2." and in this ED2IN file I've set SOIL_HYDRO_SCHEME=0.

Defaults came from googling the description of the variable with the word "typical" and "soil".

Also fixes a mistake in #3035 that added new variables not in the ED2IN template after the $END tag.

Motivation and Context

Required variables for development version of ED2 to run.

Review Time Estimate

  • Immediately
  • Within one week
  • When possible

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Extremely unlikely to break anything, and I can't test if it works until the PR gets merged and the docker container gets built by GitHub actions.

Checklist:

  • My change requires a change to the documentation.
  • My name is in the list of CITATION.cff
  • I have updated the CHANGELOG.md.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@Aariq Aariq added the Status: Ready Pull request ready for merge, or issue ready to be closed label Sep 15, 2022
@Aariq Aariq removed the Status: Ready Pull request ready for merge, or issue ready to be closed label Sep 15, 2022
@Aariq Aariq marked this pull request as draft September 15, 2022 14:17
@Aariq Aariq marked this pull request as ready for review September 15, 2022 14:49
@Aariq
Copy link
Collaborator Author

Aariq commented Sep 15, 2022

Installed on Welsh server and tested with development version of ED2. Both supplying the new template and supplying new tags with pecan.xml work.

@Aariq Aariq added the Status: Ready Pull request ready for merge, or issue ready to be closed label Sep 15, 2022
NL%SLSOC = 0.06
NL%SLPH = 6
NL%SLCEC = 0.5
NL%SLDBD = 1330
Copy link
Member

Choose a reason for hiding this comment

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

Not sure I understand why you switch out variables for hard-coded constants. Doesn't this eliminate the ability for PEcAn to set these variables?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

These are new variables that didn't exist in previous versions of ED2IN. I assumed I would need knowledge of soil chemistry to connect these up to PEcAn in some way, but if that's not the case I can take a look at how other ED2IN variables are done and take a crack at this. Also, I'm pretty sure they aren't used unless another hard-coded variable is changed.

Copy link
Member

Choose a reason for hiding this comment

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

So if the mysl* variables don't exist anywhere else in the code I'm OK with this PR, though I do think the soil.nc files do have most of these variables, if one wanted to set up the ability to specify these via code.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'll open an issue as a reminder to add this in the future

@mdietze mdietze merged commit 33ef8cb into develop Sep 28, 2022
@Aariq Aariq deleted the ed2in-defaults branch January 11, 2023 15:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Ready Pull request ready for merge, or issue ready to be closed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants