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

ENH: Dispersion Class Overhaul #543

Merged
merged 4 commits into from
Feb 8, 2024

Conversation

MateusStano
Copy link
Member

Pull request type

  • Code changes (bugfix, features)

Description

Did a big refactor on the Dispersion Class, it is working perfectly now. Simulations can be ran perfectly.

I did not touch anything relating to plots, prints or tests. So tests will not pass.

Some things for the future:

  • Something related to the saving of inputs and outputs are affecting simulation. I do not know exactly what it is
  • The whole exportables list needs to be reviewed

Next PR will be renaming all the classes.

@MateusStano MateusStano requested a review from a team as a code owner January 28, 2024 18:26
@MateusStano MateusStano self-assigned this Jan 28, 2024
@MateusStano MateusStano added the Monte Carlo Monte Carlo and related contents label Jan 28, 2024
Copy link
Collaborator

@phmbressan phmbressan left a comment

Choose a reason for hiding this comment

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

Great to see things coming to shape.

You mentioned something not working on the simulation it the PR description, in the notebook there is an error in the creation of an mc_rocket.create_object(). Is that what you meant?

rocketpy/simulation/dispersion.py Outdated Show resolved Hide resolved
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.

Good refactor here, some of the changes are already contributing to a better understanding of the class in general.

The notebook is not running to the following problem:

AttributeError: 'Dispersion' object has no attribute 'print_results'

We should keep working on the Dispersion/Monte Carlo feature and improve it to release it by version v1.3.0, hopefully soon enough!

rocketpy/simulation/dispersion.py Outdated Show resolved Hide resolved
rocketpy/simulation/dispersion.py Outdated Show resolved Hide resolved
rocketpy/simulation/dispersion.py Outdated Show resolved Hide resolved
rocketpy/simulation/dispersion.py Outdated Show resolved Hide resolved
rocketpy/simulation/dispersion.py Outdated Show resolved Hide resolved
rocketpy/simulation/dispersion.py Outdated Show resolved Hide resolved
rocketpy/simulation/dispersion.py Outdated Show resolved Hide resolved
@MateusStano
Copy link
Member Author

All comments addressed @Gui-FernandesBR @phmbressan

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.

LGTM

@Gui-FernandesBR Gui-FernandesBR added this to the Release v1.X.0 milestone Feb 7, 2024
@MateusStano MateusStano merged commit b16e329 into enh/class_dispersion Feb 8, 2024
3 of 9 checks passed
@MateusStano MateusStano deleted the enh/dispersion-model branch February 8, 2024 16:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Monte Carlo Monte Carlo and related contents
Projects
Status: Closed
Development

Successfully merging this pull request may close these issues.

3 participants