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

Importing soil thickness and root zone depth from outside the code #237

Closed
MostafaGomaa93 opened this issue Jul 1, 2024 · 11 comments
Closed
Assignees
Labels
code enhancement New feature or request

Comments

@MostafaGomaa93
Copy link
Contributor

MostafaGomaa93 commented Jul 1, 2024

Currently, the soil total thickness is fixed to 5 m in the getModelSettings (here). Same also for the depth of the root zone is fixed to 3.5 m (here). I believed that these two values can be changed from one site to another and also with different landcover, so in my opinion, it would be more convenient for the user if we can change these two values outside the code (through one of the input files).

Suggestion solution
We can add these two to the sheet input.xlsx and the user can change them from that sheet. Then, the value will be updated in the code (here)

What do you think @yijianzeng and @bobzsu? If agreed, I am willing to do this adjustment and update the code with @SarahAlidoost

@SarahAlidoost
Copy link
Member

SarahAlidoost commented Jul 1, 2024

@MostafaGomaa93 please consider adding those variables to the config file, see config_file_template.txt instead of input.xlsx. The format xlsx is proprietary and it is not recommended according to FAIR data principles.
Also, using config_file_template.txt, makes it easier to set up parameters using python interface in pystemmusscope.

@MostafaGomaa93
Copy link
Contributor Author

MostafaGomaa93 commented Jul 2, 2024

@SarahAlidoost Is there another option than the config_file? just thinking because the config file is all about the path to the data folders, and no variables are assigned in it.

@SarahAlidoost
Copy link
Member

@SarahAlidoost Is there another option than the config_file? just thinking because the config file is all about the path to the data folders, and no variables are assigned in it.

Currently variables like “Location”, “startTime” and “endTime” are also in the config_file_template. The idea of having one configuration file is to follow community standards for coupling models: a config file should be used to setup a model (see PyMT definition) or initialize a model (see BMI definition). model.setup usually creates a working directory and finds the data based on the information in the config file.

For stemmus_scope, it would be great to merge all configurations (IO directories) and tunable parameters (getModelSettings and input.xlsx, hardcoded values) into one file. This way, users are able to reproduce/reuse a workflow by changing one file instead of several files. In addition, having one file makes the development and maintenance easier.

I agree that it is better to separate directories from parameters, like by creating sections in a yaml file. However, it requires changing the format of txt to yaml and updating the reading and writing functions in stemmus_scope and pystemmusscope. For more info, see an example of yaml file to run zampy.

@MostafaGomaa93
Copy link
Contributor Author

MostafaGomaa93 commented Jul 3, 2024

Thanks for the explanation @SarahAlidoost. I indeed agree with you about having one file for all settings and parameters. In the meantime, can we do it through the BMI? if agreed, I can add these two variables to the BMI table (here). I will also make a very slight change in the source code.

I already discussed the idea with @yijianzeng and he agreed.

@SarahAlidoost
Copy link
Member

Thanks for the explanation @SarahAlidoost. I indeed agree with you about having one file for all settings and parameters. In the meantime, can we do it through the BMI? if agreed, I can add these two variables to the BMI table (here). I will also make a very slight change in the source code.

I already discussed the idea with @yijianzeng and he agreed.

yes, it can be added to the BMI implementation. But the initial values of these two variables should be defined somewhere in the stemmus_scope code.

@MostafaGomaa93
Copy link
Contributor Author

Thanks for the explanation @SarahAlidoost. I indeed agree with you about having one file for all settings and parameters. In the meantime, can we do it through the BMI? if agreed, I can add these two variables to the BMI table (here). I will also make a very slight change in the source code.
I already discussed the idea with @yijianzeng and he agreed.

yes, it can be added to the BMI implementation. But the initial values of these two variables should be defined somewhere in the stemmus_scope code.

Yes, the defalut values are already there in the code.

@SarahAlidoost
Copy link
Member

Thanks for the explanation @SarahAlidoost. I indeed agree with you about having one file for all settings and parameters. In the meantime, can we do it through the BMI? if agreed, I can add these two variables to the BMI table (here). I will also make a very slight change in the source code.
I already discussed the idea with @yijianzeng and he agreed.

yes, it can be added to the BMI implementation. But the initial values of these two variables should be defined somewhere in the stemmus_scope code.

Yes, the defalut values are already there in the code.

The variables added to the table here are in the structure ModelSettings. Currently, model settings are loaded in different scripts as ModelSettings = io.getModelSettings();, see search results. Therefore, it is not possible to update the variables of ModelSettings through BMI because calling io.getModelSettings() will reset the values again. We need a workaround for this. @BSchilperoort what do you think?

@MostafaGomaa93
Copy link
Contributor Author

MostafaGomaa93 commented Jul 5, 2024

Notices to be considered

I have done several tests in changing the total soil depth (getModelSettings.Tot_Depth). To avoid errors in the model run, the following need to be considered:

  • Minimum value that can be assigned to the getModelSettings.Tot_Depth = 300 cm. This is because of the structure of the Dtrmn_Z function. In the future releases, if we want to change the getModelSettings.Tot_Depth to be < 300, then the Dtrmn_Z will need to be adjusted.
  • Maximum value that can be assigned to the getModelSettings.Tot_Depth = 1400 cm. That is because the maximum number of soil layers getModelSettings.NL = 100. So, if we want to let getModelSettings.Tot_Depth > 1400 cm, the getModelSettings.NL will need to be > 100.
  • The following is a description of a required change (already done). The indices of the first 4 layers (layers close to land surface) are from 51 to 54 (as long as the total soil thickness equals 5 m). However, if we change the total soil thickness (not 5 m), then the indices of the first 4 layers will not be from 51 to 54 anymore. So, a simple change in these 3 files (one, two, three) to get the proper indices of the first 4 layers.

@yijianzeng
Copy link
Contributor

Currently variables like “Location”, “startTime” and “endTime” are also in the config_file_template. The idea of having one configuration file is to follow community standards for coupling models: a config file should be used to setup a model (see PyMT definition) or initialize a model (see BMI definition). model.setup usually creates a working directory and finds the data based on the information in the config file.

We can enrich the Config file with categories, following community standards: 'Directory Info,' 'Namelist,' etc.

@yijianzeng
Copy link
Contributor

For stemmus_scope, it would be great to merge all configurations (IO directories) and tunable parameters (getModelSettings and input.xlsx, hardcoded values) into one file. This way, users are able to reproduce/reuse a workflow by changing one file instead of several files. In addition, having one file makes the development and maintenance easier.

Strongly agree!

@yijianzeng
Copy link
Contributor

  • The following is a description of a required change (already done). The indices of the first 4 layers (layers close to land surface) are from 51 to 54 (as long as the total soil thickness equals 5 m). However, if we change the total soil thickness (not 5 m), then the indices of the first 4 layers will not be from 51 to 54 anymore. So, a simple change in these 3 files (one, two, three) to get the proper indices of the first 4 layers.

This is linked to issues 241

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

4 participants