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.

Sign up for GitHub

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

Jump to bottom

DOCs: improve mass and inertia docs #445

Merged
merged 15 commits into from
Nov 12, 2023
Merged

DOCs: improve mass and inertia docs #445

merged 15 commits into from
Nov 12, 2023

Conversation

Gui-FernandesBR
Copy link
Member

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) -> not needed
  • Docs have been reviewed and added / updated
  • Lint (black rocketpy/ tests/) has passed locally
  • All tests (pytest --runslow) have passed locally

Current behavior

  • It's simply too hard to understand all the inputs required by rocketpty
  • We got several reports of people confusing the measures of inertia tensors

New behavior

  • I updated all the motor and rocket related class. This consumed a very long time to mine.
  • Documentation should be clearer, any typo fixed, and more descriptions annotated.

Breaking change

  • No

Additional information

  • For the future, we need better explanations and maybe drawing in or documentation. Some people have no idea of what an inertia tensor is.

@Gui-FernandesBR Gui-FernandesBR added the Docs Docs and examples related label Oct 28, 2023
@Gui-FernandesBR Gui-FernandesBR added this to the Release v1.X.0 milestone Oct 28, 2023
@Gui-FernandesBR Gui-FernandesBR self-assigned this Oct 28, 2023
@Gui-FernandesBR Gui-FernandesBR mentioned this pull request Nov 7, 2023
Merged
3 tasks
MateusStano
MateusStano approved these changes Nov 7, 2023
View reviewed changes
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.

Really Great! Just made a few suggestions, but otherwise this is ready to merge

rocketpy/rocket/rocket.py Outdated Show resolved Hide resolved
rocketpy/rocket/rocket.py Outdated Show resolved Hide resolved
rocketpy/rocket/rocket.py Outdated Show resolved Hide resolved
rocketpy/motors/solid_motor.py Outdated Show resolved Hide resolved
@Gui-FernandesBR Gui-FernandesBR removed the request for review from giovaniceotto November 9, 2023 19:14
@Gui-FernandesBR
Copy link
Member Author

All good now.

@phmbressan could you also review this one before merging? This changes much of the motor's documentation, I'd like to have an opinion from you.

phmbressan
phmbressan approved these changes Nov 11, 2023
View reviewed changes
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.

Docs seems much clearer now. Made some small comments that I would rather to not overlook.

Note: when applying a correction, please check it out for other possible instances of the same dosctring. Its is hard to pinpoint all occurences on GitHub comments.

rocketpy/motors/hybrid_motor.py Outdated Show resolved Hide resolved
rocketpy/motors/hybrid_motor.py Show resolved Hide resolved
rocketpy/motors/liquid_motor.py Outdated Show resolved Hide resolved
rocketpy/rocket/rocket.py Outdated Show resolved Hide resolved
rocketpy/rocket/rocket.py Outdated Show resolved Hide resolved
rocketpy/utilities.py Outdated Show resolved Hide resolved
rocketpy/motors/motor.py Outdated Show resolved Hide resolved
rocketpy/rocket/rocket.py Outdated Show resolved Hide resolved
@Gui-FernandesBR
DOC: improve docs based on latest review.
8eb039d 
- Refined input parameters for apogee and liftoff speed calculations
- Inertia definition in the Rocket class
- Change the Motor.dry_mass definition
- Avoid repetition of dry_mass definitions
- Add a few for docstrings
@Gui-FernandesBR
Copy link
Member Author

All good now. Thanks for both reviews.

@Gui-FernandesBR Gui-FernandesBR merged commit f9ff423 into master Nov 12, 2023
9 checks passed
@Gui-FernandesBR Gui-FernandesBR deleted the docs/improve_mass_docs branch November 12, 2023 21:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@phmbressan phmbressan phmbressan approved these changes

@MateusStano MateusStano MateusStano approved these changes

@giovaniceotto giovaniceotto Awaiting requested review from giovaniceotto

Assignees

@Gui-FernandesBR Gui-FernandesBR

Labels
Docs Docs and examples related
Projects
LibDev Roadmap
Status: Closed
Milestone
Release v1.X.0
Development

Successfully merging this pull request may close these issues.
3 participants
@Gui-FernandesBR @MateusStano @phmbressan