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: Remove Pydantic From DispersionModel, Environment and SolidMotor #509

Merged
merged 13 commits into from
Dec 13, 2023

Conversation

MateusStano
Copy link
Member

Pull request type

  • Code changes (bugfix, features)

Checklist

  • Tests for the changes have been added (if needed)
  • Docs have been reviewed and added / updated
  • Lint (black rocketpy/ tests/) has passed locally
  • All tests (pytest --runslow) have passed locally
  • CHANGELOG.md has been updated (if relevant)

Current behavior

All the Dispersion related classes were built upon pydantic, which made some of the input validation much easier to do. However, pydantic is a huge package, and we were doing something very simple with it, using only a very small and specific part of the library.
The dependency of this library also made maintaining these classes harder for new developers

After a big hiatus from working on dispersion, pydantic updated to v2 and changing our code to fit the new versions would be a lot of work. Specially since a lot of the classes' inputs changed (dry_mass, inertias etc...)

New behavior

Pydantic has been totally removed from some of the dispersion classes. I will be making smaller Pull Requests removing pydantic for every few classes, since there are a lot of additions and changes. This way we speed up our revision process

Structure and documentation of the changed classes has also been improved

This PR only removes pydantic from:

  • DispersionModel
  • McEnvironment
  • McSolidMotor

Notes:

  • MotorDispersionModel class was created. This will be really useful for the future implementation of hybrid and liquids
  • Very few changes in DispersionModel were related to pydantic's removal. Mostly it was just improvements to the validation function structure and documentation

Additional information

  • The dispersion tests are not going to pass. That is expected
  • I looked at some other libraries (like pyfields and shema) but I did not manage to find any that suited our needs while being easier to implement than just making the classes with pure python
  • Documenting these classes are honestly harder than making them. I ask you to review them with special care
  • There are examples of usage of McEnvironment and McSolidMotor in dispersion_class_usage.ipynb, but that notebook is not even close to being completed

Questions

I have a few issues with this implementation that needs some discussing to be resolved

  • I have removed datum and time_zone from McEnvironment. It does not make sense to vary does since we do not rebuild the atmospheric model. However, this is also true for the date argument. Should we really not rebuild the atmospheric model? Adding an optional parameter to chose that to the user seems like a good solution.
  • The burn_time argument has a few issues:
    • Since burn_time can be a tuple, how could we adapt that to the dispersion classes inputs?
    • burn_time is passed directly to reshape_thrust_curve, but this only accepts burn_time in the int/float form. If we allow burn_time to be passed as a tuple somehow, how would we make it work in reshape_thrust_curve?
    • Currently in McSolidMotor, burn_time is assumed to be in the int/float format, meaning we assume that the user's thrust curve starts at time zero and ends at the burn_time. Should we keep like this?
  • I have separated the inertia components for the McSolidMotor inputs. It is a lot of arguments, but I could not think of any other way of doing it. Any suggestions?

@MateusStano MateusStano requested a review from a team as a code owner December 9, 2023 18:22
@MateusStano MateusStano self-assigned this Dec 9, 2023
@MateusStano MateusStano added the Monte Carlo Monte Carlo and related contents label Dec 9, 2023
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.

This Looks Good To Me

rocketpy/monte_carlo/mc_environment.py Outdated Show resolved Hide resolved
rocketpy/monte_carlo/dispersion_model.py Outdated Show resolved Hide resolved
rocketpy/monte_carlo/dispersion_model.py Outdated Show resolved Hide resolved
rocketpy/monte_carlo/dispersion_model.py Outdated Show resolved Hide resolved
@Gui-FernandesBR
Copy link
Member

Questions
I have a few issues with this implementation that needs some discussing to be resolved

  • I have removed datum and time_zone from McEnvironment. It does not make sense to vary does since we do not rebuild the atmospheric model. However, this is also true for the date argument. Should we really not rebuild the atmospheric model? Adding an optional parameter to chose that to the user seems like a good solution.
  • Since burn_time can be a tuple, how could we adapt that to the dispersion classes inputs?
  • burn_time is passed directly to reshape_thrust_curve, but this only accepts burn_time in the int/float form. If we allow burn_time to be passed as a tuple somehow, how would we make it work in reshape_thrust_curve?
  • Currently in McSolidMotor, burn_time is assumed to be in the int/float format, meaning we assume that the user's thrust curve starts at time zero and ends at the burn_time. Should we keep like this?
  • I have separated the inertia components for the McSolidMotor inputs. It is a lot of arguments, but I could not think of any other way of doing it. Any suggestions?

Yes, it currently do not make any sense recreating the Environment model, only if you are not sure where you want to launch and this uncertainty is about some kilometers. Just doesn't make sense.
What makes sense is to vary the ensemble members or adding a wind component.

Given you faced problem with both burn_time and intertia_tensor since they are tuples, it seems our models can't handle tuples argument yet. But you solved it for inertia tensor... Why could we do the same for the burn_time too?

It's ok make some assumptions at this stage as long as you documment it. This address the penultimate question

@Gui-FernandesBR
Copy link
Member

Tests not passing, is there anything we have to worry about it?

@MateusStano
Copy link
Member Author

Given you faced problem with both burn_time and intertia_tensor since they are tuples, it seems our models can't handle tuples argument yet. But you solved it for inertia tensor... Why could we do the same for the burn_time too?

Thats a great idea! Done in 631f46e

Tests not passing, is there anything we have to worry about it?

Nope, I have not looked at the tests yet

@Gui-FernandesBR
Copy link
Member

Important to keep private methods

@MateusStano MateusStano merged commit 1d23858 into enh/class_dispersion Dec 13, 2023
3 of 9 checks passed
@Gui-FernandesBR Gui-FernandesBR deleted the enh/remove-pydantic-dispersion branch December 15, 2023 07:18
@Gui-FernandesBR Gui-FernandesBR added this to the Release v1.X.0 milestone Feb 13, 2024
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.

None yet

2 participants