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

DOC: fix Getting Started notebooks Dynamic Analysis section #298

Merged
merged 2 commits into from
Nov 15, 2022

Conversation

giovaniceotto
Copy link
Member

Pull request type

Please check the type of change your PR introduces:

  • ReadMe, Docs and GitHub maintenance

Pull request checklist

Please check if your PR fulfills the following requirements, depending on the type of PR:

  • ReadMe, Docs and GitHub maintenance:

    • Spelling has been verified
    • Code docs are working correctly

What is the current behavior?

As reported in #292, the Dynamic Stability Analysis section of the Getting Started Notebook is not working as expected:

image

What is the new behavior?

The problem was that the variable factor was not being used inside the for loop. This has been fixed:

image

Does this introduce a breaking change?

  • Yes
  • No

Other information

Should be merged directly into master since it is not related to the core code, but to the documentation. Once merged, the documentation will be updated automatically.

@giovaniceotto giovaniceotto removed the request for review from MateusStano November 15, 2022 17:01
@giovaniceotto giovaniceotto added the Docs Docs and examples related label Nov 15, 2022
@giovaniceotto
Copy link
Member Author

@Gui-FernandesBR, please consider the importance of us merging this PR fast since the latest documentation is wrong.

@Gui-FernandesBR
Copy link
Member

Shall we use %matplotlib inline instead of widget? This way the plots are rendered in docs section, wllowing us to see what is being shown to be readers

Copy link
Member

@Gui-FernandesBR Gui-FernandesBR left a comment

Choose a reason for hiding this comment

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

Ty, awesome update

@Gui-FernandesBR Gui-FernandesBR merged commit 0238ddd into master Nov 15, 2022
@Gui-FernandesBR
Copy link
Member

@giovaniceotto ,

https://docs.rocketpy.org/en/latest/notebooks/getting_started.html

It is still using %matplotlib widget here, therefore all the plots are not rendered.
The code is correct though. And running at colab worked too.

@giovaniceotto
Copy link
Member Author

@Gui-FernandesBR, I believe the problem of not showing the plots is more related to ReadTheDocs/Sphinx. Here on GitHub, they do render correctly.

We can investigate this in the future.

@Gui-FernandesBR Gui-FernandesBR deleted the doc/fix-getting-started branch November 17, 2022 02:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Docs Docs and examples related
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants