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: motor coordinates #423

Merged
merged 12 commits into from
Oct 7, 2023
Merged

BUG: motor coordinates #423

merged 12 commits into from
Oct 7, 2023

Conversation

phmbressan
Copy link
Collaborator

Pull request type

  • Code changes (bugfix, features)
  • Code maintenance (refactoring, formatting, tests)
  • ReadMe, Docs and GitHub updates
  • Other (please describe):

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

Current behavior

A bug was identified by @Gui-FernandesBR regarding add_motor coordinate conversion. A legacy assumption of nozzle_position=0 was being considered in code.

New behavior

The coordinate conversion was fixed and now is consistent with the documentation: positions are defined with respect to the coordinate system's origin of the objects involved.

A parametrized test was added to check this behavior.

Breaking change

  • Yes
  • No

@phmbressan phmbressan added the Bug Something isn't working label Oct 3, 2023
@phmbressan phmbressan self-assigned this Oct 3, 2023
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.

Overall a simple and effective solution. I really liked it.

As this is a bug solving, I believe we should merge it directly to the master branch so we don't spend much time until actually getting these changes to the default branch.

tests/test_rocket.py Show resolved Hide resolved
tests/conftest.py Show resolved Hide resolved
rocketpy/rocket/rocket.py Show resolved Hide resolved
@phmbressan phmbressan changed the base branch from develop to master October 3, 2023 11:40
Copy link
Member

@MateusStano MateusStano left a comment

Choose a reason for hiding this comment

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

I think there are still some things wrong thorought the code. Here is what I found:

  • rocket_prints.py: Line 97,
  • rocket.py: Line 102, documentation is wrong
  • flight.py: effective_1rl and effective_2rl seem wrong
  • flight.py: b and c parameters in u_dot
  • flight.py: r_NOZ in u_dot_generalized
  • flight.py: in b parameter of rotational_energy

Btw. The effective rail cached properties were duplicated, I just deleted them here because they are still related to this bug

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.

Approved again, good work here!

@Gui-FernandesBR Gui-FernandesBR changed the title Bug/motor coordinates BUG: motor coordinates Oct 7, 2023
@Gui-FernandesBR Gui-FernandesBR merged commit c3f9a13 into master Oct 7, 2023
9 checks passed
@Gui-FernandesBR Gui-FernandesBR deleted the bug/motor-coordinates branch October 7, 2023 21:38
@Gui-FernandesBR Gui-FernandesBR added this to the Release v1.X.0 milestone Oct 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants