Add/Edit variables to BMI #116
Conversation
Co-authored-by: Bart Schilperoort <b.schilperoort@esciencecenter.nl>
|
@MostafaGomaa93 and @BSchilperoort the failed markdown-link-check might be related to this issue. I submitted issue #119. |
SarahAlidoost
left a comment
SarahAlidoost
left a comment
There was a problem hiding this comment.
@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.
BSchilperoort
left a comment
BSchilperoort
left a comment
There was a problem hiding this comment.
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", |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
What STEMMUS-SCOPE calculates is soil_evaporation and not total evaporation.
Then the evaporation variable should be named soil_evaporation!
|
closes #114
closes #115
closes #111