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 missing requirements.txt to oscillator and oscillator-overlap tutorial #556

Merged
merged 3 commits into from
Aug 29, 2024

Conversation

NiklasVin
Copy link
Collaborator

This PR adds the requirements.txt files for both python solvers of this tutorial, that were missing.

@NiklasVin NiklasVin changed the title Add missing requirements to oscillator tutorial Add missing requirements.txt to oscillator tutorial Aug 19, 2024
Copy link
Member

@MakisH MakisH left a comment

Choose a reason for hiding this comment

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

Thank you for spotting this!
I think this case is a bit special, since we have the solver in a dedicated directory.

There might be more similar cases to update accordingly.

oscillator/mass-left-python/requirements.txt Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

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

In this case, we have the solver in ../solver-python.
Would it make sense to create the requirements.txt and the venv there, to avoid duplication?

Copy link
Member

@BenjaminRodenberg BenjaminRodenberg Aug 29, 2024

Choose a reason for hiding this comment

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

I think that's a good point that could also save us a lot of implementation & maintainment work in #547.

Let's do the following here: Let's get this PR merged with the structure as it is. Moving requirements.txt and the venv into the solver is a general thing that we should discuss in #574 and then solve consistently (maybe even for the nutils cases). I'll add a comment there.

Copy link
Member

@BenjaminRodenberg BenjaminRodenberg left a comment

Choose a reason for hiding this comment

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

Maybe consider adding oscillator-overlap in this PR as well? I think everything should be pretty identical to oscillator.

We could also do this in a new PR, but if @NiklasVin is doing another round of testing before merging then I think doing oscillator-overlap in the same sweep would be a good idea.

Took care of this since I was running the case anyways. Will merge this PR now.

Copy link
Member

@BenjaminRodenberg BenjaminRodenberg left a comment

Choose a reason for hiding this comment

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

I did a quick test run of this case. After adding scipy everything works.

@BenjaminRodenberg BenjaminRodenberg changed the title Add missing requirements.txt to oscillator tutorial Add missing requirements.txt to oscillator and oscillator-overlap tutorial Aug 29, 2024
@BenjaminRodenberg BenjaminRodenberg merged commit fc35d6b into precice:develop Aug 29, 2024
1 check 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