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

Add/Edit variables to BMI #116

Merged
merged 21 commits into from
Dec 20, 2024
Merged

Add/Edit variables to BMI #116

merged 21 commits into from
Dec 20, 2024

Conversation

MostafaGomaa93
Copy link
Contributor

@MostafaGomaa93 MostafaGomaa93 commented Nov 28, 2024

closes #114
closes #115
closes #111

@SarahAlidoost
Copy link
Member

@MostafaGomaa93 and @BSchilperoort the failed markdown-link-check might be related to this issue. I submitted issue #119.

Copy link
Member

@SarahAlidoost SarahAlidoost left a comment

Choose a reason for hiding this comment

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

@MostafaGomaa93 thanks for adding new variables to bmi and fixing some issues. The changes look good and the notebook bmi_demo works fine. The documentation about SleepDuration will be added in #118 where I am updating docs.

Copy link
Contributor

@BSchilperoort BSchilperoort left a comment

Choose a reason for hiding this comment

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

Looks largely OK. Just have a comment on a variable name.

Can you also list the newly available variables in the changelog?

keys=["Evap"],
),
BmiVariable(
name="transpiration_total",
Copy link
Contributor

Choose a reason for hiding this comment

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

I can understand the "evaporation_total" name, as transpiration is separate (and you can have soil evaporation separately as well) but why does transpiration have _total in the name?

You cannot get transpiration_understory and transpiration_canopy right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I actually agree with you, I was trying to give a similar name to evaporation_total. However, I don't get why it is called evaporation_total. What STEMMUS-SCOPE calculates is soil_evaporation and not total evaporation. Physically, there is still one evaporation component that is missing (called canopy interception), but it is not simulated by STEMMUS_SCOPE yet.

So maybe we should change the names
evaporation_total to evaporation
transpitation_total to transpiration

Copy link
Contributor

Choose a reason for hiding this comment

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

What STEMMUS-SCOPE calculates is soil_evaporation and not total evaporation.

Then the evaporation variable should be named soil_evaporation!

@MostafaGomaa93 MostafaGomaa93 merged commit 43c5e60 into main Dec 20, 2024
16 of 17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants