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

Copy soil layer setting csv to input folder + add a BMI MODFLOW coupling notebook #104

Merged
merged 36 commits into from
Sep 27, 2024

Conversation

MostafaGomaa93
Copy link
Contributor

@MostafaGomaa93 MostafaGomaa93 commented Aug 21, 2024

relates #103

🔴 This branch is based on the branch bmi-groundwater-coupling in PR #101, should be merged after that.

Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

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 Well done 👍 , nice to see the complete workflow of coupling. Thanks for uploading the required files to Zenodo; it's great to see that the example data for NL-Loo is now available. I successfully ran the notebook using the NL-Loo example data, but I cannot run the notebook for a different site. Because I couldn’t find any instructions on this in the introductory cell of the notebook or within the files uploaded to Zenodo. Can you please add the instructions here in the notebook or/and a preprocessing script to zenodo?

- In the intro section of your notebook:

  1. Replace the link to the documentation of pyStemmusScope package with the latest link as https://pystemmusscope.readthedocs.io/en/latest/.
  2. Provide the exact link to STEMMUS_SCOPE executable file and not the repository e.g. https://github.com/EcoExtreML/STEMMUS_SCOPE/blob/main/run_model_on_snellius/exe/STEMMUS_SCOPE
  3. The same for MODFLOW 6 executable files, provide the exact links.

- The paths to the files:

  1. The codes in cells 1, 1.1, and 1.2 are difficult to read and follow. Additionally, the notebook doesn't need to handle tasks like copying files for users. Instead, users should simply be required to specify the following items:
modflow_working_directory = "path_to_modflow_dir/mf6_model_base"
modflow_config_file = f"{modflow_working_directory}/mfsim.nam"
modflow_exe_file = "path_to_modelfolw_executables/libmf6.so"  # for window .dll, otherwise .so
stemmus_scope_config_file = "path_to_stemmus_scope_config_file/config.txt"
  1. Accordingly, please rename the corresponding variables as:
os.environ["STEMMUS_SCOPE"] = read_config(stemmus_scope_config_file)["ExeFilePath"]

In cell 2.1,

stsc.initialize(stemmus_scope_config_file)

In cell 2.2.,

mf6 = ModflowApi(modflow_exe_file, working_directory = modflow_working_directory)
mf6.initialize(modflow_config_file)
  1. To help users in locating the required files, in the introduction section of your notebook, when mentioning the Zenodo entry, please include the relative paths to the necessary items and tell users to copy these files to their own working directory. For example, modflow_working_directory = ./SSM_examples/ex1/mf6_model_base

-The variable names in all cells of the notebook:
please use clear and readable names in the code. It's better to have a longer, more descriptive name than a short, unclear one. For example, instead of stsc, consider using stemmus_scope or stemmus_scope_model.

-RuntimeError
I'm not sure why a RuntimeError should be raised during the model finalization process. If there's an issue, finalizing the models should be handled in separate cells. Otherwise, if an error occurs in modflow, for example, it will prevent stemmus_scope from being finalized properly.

-Documentation:
Please add the notebook to the documentation, by adding an item as - "example: coupling modflow and stemmus_scope": notebooks/bmi_MODFLOW_coupling.ipynb in the BMI section in mkdocs.yml file.

If something is not clear, please let me know.

@MostafaGomaa93
Copy link
Contributor Author

MostafaGomaa93 commented Aug 25, 2024

If something is not clear, please let me know.

Many thanks @SarahAlidoost for the comments and suggestions. I did all of them and updated the notebook. Have a look again please at the notebook and let me know if anything is still required.

@SarahAlidoost
Copy link
Member

If something is not clear, please let me know.

Many thanks @SarahAlidoost for the comments and suggestions. I did all of them and updated the notebook. Have a look again please at the notebook and let me know if anything is still required.

Thanks for addressing the comments. I saw the link to https://flopy.readthedocs.io/en/stable/tutorials.html#modflow-6. I created a model simulation using the code block in https://flopy.readthedocs.io/en/stable/Notebooks/mf6_data_tutorial01.html. However, I couldn't run the coupling notebook with the modflow files because some parameters needed to be adjusted. Can you please add a complete code block to your coupling notebook that creates the same mf6_model_base of your example?

@SarahAlidoost SarahAlidoost mentioned this pull request Sep 23, 2024
@SarahAlidoost
Copy link
Member

@MostafaGomaa93 can you please merge the main branch to your branch in this pull request? So, the merge conflicts are resolved.

@MostafaGomaa93
Copy link
Contributor Author

@MostafaGomaa93 can you please merge the main branch to your branch in this pull request? So, the merge conflicts are resolved.

Done

Copy link

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 the notebook that shows the coupling workflow. Nice addition to the documentation 👍 I updated the docker versions and added some tests for new changes to config_io. Ready to merge.

@SarahAlidoost SarahAlidoost merged commit 33adb9e into main Sep 27, 2024
16 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
Development

Successfully merging this pull request may close these issues.

3 participants