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: Add reference area factor correction to aero surfaces (solves #557) #558

Merged
merged 10 commits into from
Feb 22, 2024

Conversation

giovaniceotto
Copy link
Member

@giovaniceotto giovaniceotto commented Feb 14, 2024

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 tests -m slow --runslow) have passed locally
  • CHANGELOG.md has been updated (if relevant)

Current behavior

See #557.

New behavior

Issue #557 is fixed.

Breaking change

  • No

TODO:

  • Add tests to prevent similar issues from going undetected again.
  • Update CHANGELOG.md.
  • Bump up version to 1.2.1 and release hotfix. (Gui: different PR maybe?)

Copy link

codecov bot commented Feb 14, 2024

Codecov Report

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

Comparison is base (d4ac946) 72.47% compared to head (643ea6e) 72.58%.
Report is 1 commits behind head on master.

Files Patch % Lines
rocketpy/rocket/aero_surface.py 85.71% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #558      +/-   ##
==========================================
+ Coverage   72.47%   72.58%   +0.10%     
==========================================
  Files          59       59              
  Lines        9567     9576       +9     
==========================================
+ Hits         6934     6951      +17     
+ Misses       2633     2625       -8     

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

@Gui-FernandesBR Gui-FernandesBR added Bug Something isn't working Aerodynamics Any problem to be worked on top of RocketPy's Aerodynamic labels Feb 14, 2024
@Gui-FernandesBR Gui-FernandesBR added this to the Release v1.X.0 milestone Feb 14, 2024
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.

Thank you so much for a fast resolution for this bug. Here comes my first review.

I will try to help with the remaining TODOs, let's coordinate it together.

I would appreciate hearing from you on the comments that I've added below.

rocketpy/prints/rocket_prints.py Show resolved Hide resolved
rocketpy/rocket/rocket.py Outdated Show resolved Hide resolved
rocketpy/rocket/aero_surface.py Outdated Show resolved Hide resolved
rocketpy/rocket/rocket.py Show resolved Hide resolved
rocketpy/rocket/rocket.py Outdated Show resolved Hide resolved
rocketpy/prints/rocket_prints.py Show resolved Hide resolved
@Gui-FernandesBR Gui-FernandesBR changed the title HOTFIX: Issue #557 HOTFIX: Add reference area factor correction to aero surfaces (solves #557) Feb 19, 2024
@Gui-FernandesBR Gui-FernandesBR changed the title HOTFIX: Add reference area factor correction to aero surfaces (solves #557) BUG: Add reference area factor correction to aero surfaces (solves #557) Feb 19, 2024
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.

Let's open an issue to deal with the AeroComponent and ref_factor thread.

@lucasfourier lucasfourier self-requested a review February 19, 2024 19:46
tests/test_rocket.py Outdated Show resolved Hide resolved
lucasfourier
lucasfourier previously approved these changes Feb 19, 2024
Copy link
Contributor

@lucasfourier lucasfourier left a comment

Choose a reason for hiding this comment

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

Great work guys. The only thing left to do I guess would be to refactor the newly created tests. Gonna open an issue for that. I think there is room for creating fixtures, parametrizing, etc. However, for a hotfix this is more than enough.

@Gui-FernandesBR Gui-FernandesBR dismissed stale reviews from lucasfourier and themself via 643ea6e February 19, 2024 22:39
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.

LGTM,

All credits go to @giovaniceotto (aka the wizard) for this fix.

@phmbressan
Copy link
Collaborator

Thanks for quickly solving the review comments @Gui-FernandesBR . Great fix by @giovaniceotto .

@Gui-FernandesBR Gui-FernandesBR merged commit ee49409 into master Feb 22, 2024
11 checks passed
@Gui-FernandesBR Gui-FernandesBR deleted the hotfix/issue-557 branch February 22, 2024 19:40
Gui-FernandesBR added a commit that referenced this pull request Feb 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Aerodynamics Any problem to be worked on top of RocketPy's Aerodynamic Bug Something isn't working
Projects
Status: Closed
Development

Successfully merging this pull request may close these issues.

BUG: Invalid Center of Pressure for Rockets with Variable Diameter
4 participants