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

DOC: Faraday 2023 flight sim #734

Merged
merged 11 commits into from
Nov 23, 2024
Merged

Conversation

LUCKIN13
Copy link
Contributor

Pull request type

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

Checklist

  • 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)

Additional information

This branch still needs some data reviews since the simulation file was developed in an older version of RocketPy. However, reviews can already be made, since the example should only suffer minor changes, if needed.
Changelog and Index for examples will be updated in other flight example, to prevent conflicts.

@LUCKIN13 LUCKIN13 requested a review from a team as a code owner November 20, 2024 21:57
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.

Really nice addition. I have a few comments to share:

  1. Can you check with the team (maybe reading their technical report) if you could improve the drag curves at least by adding a few more points? Even openrocket drag curve would already be beneficial to us. A constant value is not really helping here.
    image

  2. Also, the altitude of ejection of the main parachute seems to not be right. Any chance this value was different?

  3. Please add the new flight to the https://docs.rocketpy.org/en/latest/examples/index.html page.

  4. As Faraday is a team and not a rocket, I'm afraid we should change the name of the folder you have created. Probably this will not be the last rocket from faraday that we add to our docs.

@Gui-FernandesBR Gui-FernandesBR mentioned this pull request Nov 20, 2024
7 tasks
@LUCKIN13
Copy link
Contributor Author

LUCKIN13 commented Nov 20, 2024

Thanks @Gui-FernandesBR for your review. Valid points, almost totally fixed.
In terms of the drag data, that's the only data we have. I'll try contacting the team, to see if we can improve that example!

@Gui-FernandesBR
Copy link
Member

Thanks @Gui-FernandesBR for your review. Valid points, almost totally fixed. In terms of the drag data, that's the only data we have. I'll try contacting the team, to see if we can improve that example!

Understood.

I noticed the power_on drag is higher than the power_off drag. It should be the opposite tho.
If you swap those files, does the results get better or worse? If worse, just forget about what I said.

@LUCKIN13
Copy link
Contributor Author

@Gui-FernandesBR just went with the higher value in drag. 30 meters reduction, and I believe that since is a constant drag, it's difficult to reach better results. This can be updated later, if the team gives us more data.

@Gui-FernandesBR
Copy link
Member

@Gui-FernandesBR just went with the higher value in drag. 30 meters reduction, and I believe that since is a constant drag, it's difficult to reach better results. This can be updated later, if the team gives us more data.

Great work. That's what I expected.
5% is already really nice.

Off-topic, I believe GitHub has solved a long-known-issue which is the notebook diffs.

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.

LGTM, another amazing example added to our docs!

@LUCKIN13 go ahead and merge this PR.
I recommend using the Squash and merge option since the PR had several adjustment commits.
You have done an amazing job here.

@Gui-FernandesBR Gui-FernandesBR merged commit 09507ec into develop Nov 23, 2024
1 check passed
@Gui-FernandesBR Gui-FernandesBR deleted the docs/faraday-2023-flight-sim branch November 23, 2024 01:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Closed
Development

Successfully merging this pull request may close these issues.

2 participants