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: Eliminating multiple plots for inertia components #566

Merged

Conversation

dipesh2212
Copy link
Contributor

Pull request type

  • Code changes (bugfix, features)

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

  • No

@Gui-FernandesBR
Copy link
Member

I'm still investigating the reason, but I got this error when running my_solid_motor_example.all_info():

image

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.

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.

@Gui-FernandesBR Gui-FernandesBR added Motors Every propulsion related issue or PR Outputs Dedicated to visualizations enhancements like prints and plots labels Mar 4, 2024
@Gui-FernandesBR Gui-FernandesBR added this to the Release v1.X.0 milestone Mar 4, 2024
@Gui-FernandesBR Gui-FernandesBR linked an issue Mar 4, 2024 that may be closed by this pull request
@Gui-FernandesBR Gui-FernandesBR force-pushed the 527-eliminating-multiple-plots-for-inertia_tensors branch from 4a9912b to 81e92c6 Compare May 30, 2024 20:47
@Gui-FernandesBR
Copy link
Member

A quick update in the function:

image

I'm getting this result:
image

@Gui-FernandesBR Gui-FernandesBR changed the title eliminating multiple plots for inertia tensors ENH: Eliminating multiple plots for inertia compnents Jun 29, 2024
@Gui-FernandesBR
Copy link
Member

Here is the final result:

image

@Gui-FernandesBR Gui-FernandesBR changed the base branch from master to develop June 29, 2024 22:05
@Gui-FernandesBR Gui-FernandesBR changed the title ENH: Eliminating multiple plots for inertia compnents ENH: Eliminating multiple plots for inertia components Jun 29, 2024
Copy link

codecov bot commented Jun 29, 2024

Codecov Report

Attention: Patch coverage is 86.36364% with 3 lines in your changes missing coverage. Please review.

Project coverage is 71.86%. Comparing base (622ae81) to head (cd883da).
Report is 1 commits behind head on develop.

Current head cd883da differs from pull request most recent head 5c8e11a

Please upload reports for the commit 5c8e11a to get more accurate results.

Files Patch % Lines
rocketpy/plots/motor_plots.py 84.21% 3 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop     #566      +/-   ##
===========================================
- Coverage    71.92%   71.86%   -0.07%     
===========================================
  Files           70       70              
  Lines        10334    10320      -14     
===========================================
- Hits          7433     7416      -17     
- Misses        2901     2904       +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

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 solves the issue!

@Gui-FernandesBR Gui-FernandesBR merged commit 317579a into develop Jun 29, 2024
8 checks passed
@Gui-FernandesBR Gui-FernandesBR deleted the 527-eliminating-multiple-plots-for-inertia_tensors branch June 29, 2024 22:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Motors Every propulsion related issue or PR Outputs Dedicated to visualizations enhancements like prints and plots
Projects
Status: Closed
Development

Successfully merging this pull request may close these issues.

ENH: plot inertia tensor components in a single plot
3 participants