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 a new csv to import soil layers settings #243

Merged
merged 40 commits into from
Aug 20, 2024

Conversation

MostafaGomaa93
Copy link
Contributor

@MostafaGomaa93 MostafaGomaa93 commented Jul 25, 2024

Description

relates #237
relates #241
relates #244
relates EcoExtreML/STEMMUS_SCOPE_Processing#103

Checklist

  • Add a reference to related issues.
  • @mentions of the person or team responsible for reviewing proposed changes.
  • This pull request has a descriptive title.
  • Code is written according to the guidelines.
  • The checks by MISS_HIT style checker and linter, below the pull request, are successful (green).
  • Documentation is available.
  • Add changes to the changelog file under section Unreleased.
  • Model runs successfully.
  • Ask a meinatainer to re-generate exe file if matlab codes are changed. About how to create an exe file, see exe readme.

@MostafaGomaa93
Copy link
Contributor Author

MostafaGomaa93 commented Jul 25, 2024

@SarahAlidoost
I think the +groundwater\calculateSoilLayerThickness.m function is more related to soil settings than the groundwater calculations. Do you think it is better to move those lines to the getModelSettings.m after this line?. but, then, we will need to copy them also after this line here when the new csv file is activated

@MostafaGomaa93
Copy link
Contributor Author

MostafaGomaa93 commented Jul 25, 2024

@BSchilperoort
Do I need to explicit the exact variables needed from those two structures (ForcingData and gwfluxes) in the code? I already did that in the additional table in my comment here, but in the code yet.

@MostafaGomaa93
Copy link
Contributor Author

MostafaGomaa93 commented Jul 27, 2024

@SarahAlidoost
where should I put the new csv file? I attached here input_soilLayThick.csv. I think also the PyStemmusScope will need to be updated to recognize this new file, right?

src/STEMMUS_SCOPE.m Outdated Show resolved Hide resolved
src/STEMMUS_SCOPE.m Outdated Show resolved Hide resolved
src/STEMMUS_SCOPE.m Outdated Show resolved Hide resolved
@SarahAlidoost
Copy link
Member

@SarahAlidoost I think the +groundwater\calculateSoilLayerThickness.m function is more related to soil settings than the groundwater calculations. Do you think it is better to move those lines to the getModelSettings.m after this line?. but, then, we will need to copy them also after this line here when the new csv file is activated

For readability, let's keep the calculations in its own function calculateSoilLayerThickness. This function can be placed either in the folder +io or in a new folder like +soil.

@SarahAlidoost
Copy link
Member

@SarahAlidoost where should I put the new csv file? I attached here input_soilLayThick.csv. I think also the PyStemmusScope will need to be updated to recognize this new file, right?

The csv file is a small enough to be included in the github repository. You can add it to a new folder named example_data in the root directory. Also, please add one line in STEMMUS_SCOPE_on_CRIB.md and STEMMUS_SCOPE_on_Snellius.md under section Dataflow to explain where numbers in csv file come from and how users can use the csv file, for example, they have to copy this file to the model's input directory, ....

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 fixing the ModelSettings. See my comments.

@MostafaGomaa93
Copy link
Contributor Author

You can add it to a new folder named example_data in the root directory. Also, please add one line in STEMMUS_SCOPE_on_CRIB.md and STEMMUS_SCOPE_on_Snellius.md under section Dataflow to explain where numbers in csv file come from and how users can use the csv file, for example, they have to copy this file to the model's input directory, ....

Okay, I add an explanation as you suggest. Please have a look and let me know if it is clear or something is still missing

@SarahAlidoost
Copy link
Member

@Crystal-szj this pull request refactors the codes of ModelSettings, related to your issue #246. Now ModelSettings = io.getModelSettings() is not called in several scripts but instead, the input argument ModelSettings is passed as an input argument to the functions requiring model settings. This way, the ModelSettings can be changed from outside of the model. I have already reviewed this pull request. Can you please help to re-generate exe file on snellius using this branch? thanks.

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 the implementation 👍 . Once exe file is added, it can be merged.

@Crystal-szj
Copy link
Contributor

@Crystal-szj this pull request refactors the codes of ModelSettings, related to your issue #246. Now ModelSettings = io.getModelSettings() is not called in several scripts but instead, the input argument ModelSettings is passed as an input argument to the functions requiring model settings. This way, the ModelSettings can be changed from outside of the model. I have already reviewed this pull request. Can you please help to re-generate exe file on snellius using this branch? thanks.

Sure. I'll recreate it and let you know.

@Crystal-szj
Copy link
Contributor

Crystal-szj commented Aug 19, 2024

Hi @SarahAlidoost @MostafaGomaa93, I have re-generated the executable file. The merging is still blocked. Should I merge this branch into the main branch directly?

@SarahAlidoost
Copy link
Member

SarahAlidoost commented Aug 20, 2024

Hi @SarahAlidoost @MostafaGomaa93, I have re-generated the executable file. The merging is still blocked. Should I merge this branch into the main branch directly?

@Crystal-szj Great, Thanks 👍 the reason that merging is blocked is that the pull request is waiting for the approval of reviewers (in this case you and me 😄 ). I will go ahead with the pull request.

@SarahAlidoost
Copy link
Member

@MostafaGomaa93 and @Crystal-szj I noticed that the model run isnot successful. I fixed the code in this commit and re-generate exe file.

@SarahAlidoost SarahAlidoost merged commit 5d24628 into main Aug 20, 2024
1 check passed
@MostafaGomaa93
Copy link
Contributor Author

@MostafaGomaa93 and @Crystal-szj I noticed that the model run isnot successful. I fixed the code in this commit and re-generate exe file.

Thanks @SarahAlidoost for this leftover fix

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants