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

Update outdated docs #353

Merged
merged 25 commits into from
Jul 27, 2023
Merged

Update outdated docs #353

merged 25 commits into from
Jul 27, 2023

Conversation

sverhoeven
Copy link
Member

@sverhoeven sverhoeven commented Jun 13, 2023

Refs #350

TODO

  • docstrings
    • forcings
    • parameter sets
    • models
  • user guide notebook
    • until model run
    • generate
    • model run
  • example notebooks
    • model specific, should be moved to docs/examples/plugins//*.ipynb similar to src/ewatercycle/plugins and tests/plugins
    • forcing generate notebook: each model section should be move to docs/examples/plugins//forcing.ipynb
    • Irrigation.ipynb
  • docs on how to add model Do later in PR for Simplify adding models #338
    • I am writing a new model
    • I already have my own model
  • hpc2cluster docs
  • show that specific model has more methods. Either by adding link to super class or showing inherited methods.

@sverhoeven sverhoeven changed the title Nest hpc2cluster docs Update outdated docs Jun 13, 2023
@sverhoeven
Copy link
Member Author

@Peter9192 @BSchilperoort In the notebook we are printing objects.
Pydantic models are printed as a long single line, which is not very user friendly.
We could use https://python-devtools.helpmanual.io/ or https://github.com/willmcgugan/rich or https://docs.python.org/3/library/pprint.html (+.dict()).

To me rich looks nicest in a notebook, which do you prefer or leave as is?

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@BSchilperoort
Copy link
Member

@Peter9192 @BSchilperoort In the notebook we are printing objects. Pydantic models are printed as a long single line, which is not very user friendly. We could use https://python-devtools.helpmanual.io/ or https://github.com/willmcgugan/rich or https://docs.python.org/3/library/pprint.html (+.dict()).

To me rich looks nicest in a notebook, which do you prefer or leave as is?

The rich prints look good! It's also a very widely used package, so I see no reason why we wouldn't want to use it.

@Peter9192
Copy link
Collaborator

I've also used rich before, quite like it

@sverhoeven
Copy link
Member Author

rich it is

@sverhoeven
Copy link
Member Author

sverhoeven commented Jun 27, 2023

Notebooks where executed in a vagrant box

@sverhoeven
Copy link
Member Author

sverhoeven commented Jun 28, 2023

The wlfow get_grid_shape() seems to return inverted order (x,y instead of y,x acco) which is used to reserve numpy array. Causing things to not fit.

# bypass OptionalDestBmi and MemoizedBmi by calling origin.origin.
model_instance.bmi.origin.origin.get_grid_shape(1, np.empty((2,), dtype=np.float64))
array([169., 187.])
# Below works
model_instance.bmi.origin.origin.get_grid_x(1, np.empty((169,), dtype=np.float64))
model_instance.bmi.origin.origin.get_grid_y(1, np.empty((187,), dtype=np.float64))
# but OptionalDestBmi uses 187 for x and 169 for y, which fails.

We might needs some corrective bmi class in pyewatercycle that can swap result of get_grid_shape() or fix in container image, which might break other code.

@BSchilperoort
Copy link
Member

The wlfow get_grid_shape() seems to return inverted order (x,y instead of y,x acco) which is used to reserve numpy array. Causing things to not fit.

Looks like the problem occurs here:
https://github.com/openstreams/wflow/blob/bdc9b7966a0caa3c826b3a1f25150186b37d683f/wflow/wflow_bmi.py#L1182-L1197

The return statement should be

return [dim[5], dim[4]]

Seeing as the old wflow is not being developed anymore, I would vote for the fix with the least amount of work. If it can be easily patched in this repo, that might be best.

@BSchilperoort
Copy link
Member

I mentioned this issue during standup, and @SarahAlidoost pointed out that there was an old issue discussing the switched x/y coordinates: #104

@sverhoeven
Copy link
Member Author

Not all example notebooks have been run completely. So some old output is still there.

@sonarcloud
Copy link

sonarcloud bot commented Jul 5, 2023

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 6 Code Smells

67.1% 67.1% Coverage
1.3% 1.3% Duplication

idea Catch issues before they fail your Quality Gate with our IDE extension sonarlint SonarLint

@sverhoeven sverhoeven marked this pull request as ready for review July 5, 2023 12:40
@sverhoeven sverhoeven requested a review from Peter9192 July 5, 2023 12:40
@sverhoeven sverhoeven mentioned this pull request Jul 26, 2023
7 tasks
Copy link
Collaborator

@Peter9192 Peter9192 left a comment

Choose a reason for hiding this comment

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

This PR contains many good changes. We will need to do several more passes through the documentation as we're updating the model and forcing API, so I'm gonna approve this as so we can use is as a good basis for further improvements in follow-up PRs

@Peter9192 Peter9192 merged commit c2655f6 into main Jul 27, 2023
@Peter9192 Peter9192 deleted the 350-update-outdated-docs branch July 27, 2023 08:19
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