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

Clear up DelftBlue tutorial #359

Merged
merged 2 commits into from
May 24, 2024
Merged

Clear up DelftBlue tutorial #359

merged 2 commits into from
May 24, 2024

Conversation

EwoutH
Copy link
Collaborator

@EwoutH EwoutH commented May 23, 2024

A few minor clarifications to the DelftBlue part of the MPI tutorial.

@EwoutH EwoutH added the docs label May 23, 2024
@EwoutH EwoutH requested a review from quaquel May 23, 2024 17:11
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@EwoutH
Copy link
Collaborator Author

EwoutH commented May 24, 2024

I added a --upgrade argument to the pip install ema_workbench in the tutorial.

Test failures are unrelated, this PR is docs only.

Copy link
Owner

@quaquel quaquel left a comment

Choose a reason for hiding this comment

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

looks good

Copy link
Owner

Choose a reason for hiding this comment

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

what's the reason for these changes? It's just to make sure we use the modern super call? but if so, it should be its own (maintenance) PR?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The super stuff points.py was automatically added by pre-commit.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Rebased, after the merge of #357 it's now gone from this PR.

Copy link
Owner

Choose a reason for hiding this comment

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

ok, excellent. LGTM unless you expect further changes.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

For this PR, this is it.

Copy link
Owner

Choose a reason for hiding this comment

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

LGTM

A few minor clarifications to the DelftBlue part of the MPI tutorial.
This makes sure users don't accidently keep using an older version if they installed it previously.
@EwoutH EwoutH merged commit ce7261a into master May 24, 2024
9 of 11 checks passed
@EwoutH EwoutH deleted the mpi-tutorial branch May 24, 2024 13:35
EwoutH added a commit that referenced this pull request May 24, 2024
* Clear up DelftBlue tutorial
A few minor clarifications to the DelftBlue part of the MPI tutorial.

* MPI tutorial: pip upgrade existing EMAworkbench installation

This makes sure users don't accidently keep using an older version if they installed it previously.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants