ENH: Remove Pydantic from Remaining Classes #512
Conversation
github-actions
bot
requested review from
giovaniceotto,
Gui-FernandesBR and
phmbressan
There was a problem hiding this comment.
Good, certainly the code is longer without Pydantic, but as least we don't have to add another dependency. You made a good job with all these hard validations.
Side-note before approval, you should check out the comment regarding the components.get_by_type removal, I also think it is a breaking change.
|
There is one thing about this PR that I just noticed that is is worth thinking about. In general, This is not a huge problem for now, since I think rocketpy build in |
| @@ -138,18 +208,21 @@ def __init__( | |||
| center_of_dry_mass_position=center_of_dry_mass_position, | |||
| nozzle_position=nozzle_position, | |||
| throat_radius=throat_radius, | |||
| interpolate=None, | |||
| coordinate_system_orientation=None, | |||
There was a problem hiding this comment.
I think nobody will ever change the coordinate_system_orientation in a Monte Carlo loop, but I'm not sure what were your intentions here so just double check it please.
There was a problem hiding this comment.
Yep, these lines are guaranteeing that the interpolation and coordinat_system_orientation will be gotten from the original object. Note that these arguments are not on the class' constructor
Pull request type
New behavior
All Stochastic classes have now been updated to the new architecture.
This can be tested in the dispersion usage notebook.
Nothing has been changed in the
Dispersionclass yet.Additional Information
Next dispersion related PR will be changing all the classes names and file structure