-
-
Notifications
You must be signed in to change notification settings - Fork 135
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
eliminating multiple plots for inertia tensors #566
base: master
Are you sure you want to change the base?
eliminating multiple plots for inertia tensors #566
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The submitted code in this PR doesn't seem to be working.
@dipesh2212 we should work on
Here are some comments to guide you:
- Try to read @juliomachad0 's solution, you might benefit from his approach in this case.
- The lines like
self.I_11(*self.motor.burn_time)
in the motor_plots.py file will no longer work, please update the_MotorPlots.all
method. - Always run the tests before submitting your code, this prevents you from submitting a code that is not working.
- I think you could easily use the
Function.compare_plots
to easily create a single figure with all the inertia Functions and then display them.
Also, some other "small but important" things that I noticed:
- Rename your PR to start with
ENH:
, thus showing clearly the intention of your PR. - Please run black before submitting your code. (see the Makefile in the root directory for more details)
- If possible, please also provides screenshot of the plots you have modified, this allows us to understand what is the expected behavior without needing to run the code on our own.
def I_23(self, lower_limit=None, upper_limit=None): | ||
"""Plots I_23 of the motor as a function of time. | ||
for label in labels: | ||
getattr(self.motor, label).plot( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this will going to work as expected.
- The
Function.plot
method does not have alabel
argument. I think you should use theFunction.plot_1d
instead - This for loop is simply plotting all the curves one by one in different plots, right? Instead, what we expect from this PR is something very similar to this: https://github.com/RocketPy-Team/RocketPy/pull/547/files#
None | ||
""" | ||
self.motor.I_23.plot(lower=lower_limit, upper=upper_limit) | ||
return fig |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need to return the matplotlib fig. Just use the plt.show() instead.
4a9912b
to
81e92c6
Compare
…ttps://github.com/RocketPy-Team/RocketPy into 527-eliminating-multiple-plots-for-inertia_tensors
Pull request type
Checklist
Current behavior
There are multiple inertial tensor plots that are being plotted, it is kinda annoying to deal with those
New behavior
I've tried to integrate all the plots in one single plot which can compare the inertia tensor in a convenient way
Breaking change