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

BUG: fin_flutter_analysis doesn't find any fin set #510

Merged
merged 4 commits into from
Dec 20, 2023

Conversation

Gui-FernandesBR
Copy link
Member

@Gui-FernandesBR Gui-FernandesBR commented Dec 14, 2023

Pull request type

  • Code changes (bugfix, features)
  • Code maintenance (refactoring, formatting, tests)

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

This bug was reported through discord yesterday:
image

Also, the tests for the fin_flutter_analysis were deactivated since they were not passing.

New behavior

  • Solved the bug
  • Refactored the code to improve readability.
  • Improved the tests for flutter analysis.

Breaking change

  • No

Additional information

This feature works only for trapezoidal fins, that's said.
Also, we should improve the documentation in the future, try to give some more examples.

@Gui-FernandesBR
Copy link
Member Author

I am still working on unit tests to prevent from future occasions. Please don't review it yet

Copy link

codecov bot commented Dec 14, 2023

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (f1367e3) 71.30% compared to head (5390478) 72.02%.
Report is 2 commits behind head on develop.

❗ Current head 5390478 differs from pull request most recent head 6f4c3c3. Consider uploading reports for the commit 6f4c3c3 to get more accurate results

Files Patch % Lines
rocketpy/utilities.py 88.23% 2 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop     #510      +/-   ##
===========================================
+ Coverage    71.30%   72.02%   +0.72%     
===========================================
  Files           56       56              
  Lines         9374     9370       -4     
===========================================
+ Hits          6684     6749      +65     
+ Misses        2690     2621      -69     
Flag Coverage Δ
unittests 72.02% <88.23%> (+0.72%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

@Gui-FernandesBR Gui-FernandesBR added this to the Release v1.X.0 milestone Dec 14, 2023
@Gui-FernandesBR Gui-FernandesBR added Bug Something isn't working Tests Regarding Tests Refactor labels Dec 14, 2023

# First, we need identify if there is at least a fin set in the rocket
for aero_surface in flight.rocket.aerodynamic_surfaces:
Copy link
Member Author

Choose a reason for hiding this comment

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

Just to make reviewer's life easier: this was the problematic code line. The aerodynamic_surfaces is no longer a list of surfaces, instead it is a list of components;

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.

Good to see these tests reintroduced, let's hope they don't give the same headaches as before.

rocketpy/utilities.py Outdated Show resolved Hide resolved
@Gui-FernandesBR Gui-FernandesBR changed the title BUG: Refactor fin_flutter_analysis function to handle multiple fin sets BUG: fin_flutter_analysis doesn't find any fin set Dec 20, 2023
@Gui-FernandesBR Gui-FernandesBR merged commit 086a924 into develop Dec 20, 2023
10 checks passed
@Gui-FernandesBR Gui-FernandesBR deleted the bug/fix-fin-flutter branch December 20, 2023 01:02
@Gui-FernandesBR Gui-FernandesBR linked an issue Dec 25, 2023 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working Refactor Tests Regarding Tests
Projects
Status: Closed
Development

Successfully merging this pull request may close these issues.

TST: FinFlutterAnalysis not working properly?
2 participants