X Tutup
Skip to content

ENH: create FlightDataImporter class#474

Merged
Gui-FernandesBR merged 14 commits intodevelopfrom
enh/compare-flight-data
Dec 18, 2023
Merged

ENH: create FlightDataImporter class#474
Gui-FernandesBR merged 14 commits intodevelopfrom
enh/compare-flight-data

Conversation

@Gui-FernandesBR
Copy link
Member

@Gui-FernandesBR Gui-FernandesBR commented Nov 20, 2023 •
edited
Loading

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 --runslow) have passed locally

Current behavior

See issue #284 .

New behavior

I created this DataFrameToFlight class that I'd like to discuss before going to develop. I'm still struggling to find the best architecture for this, but at least it is a first step.

Breaking change

  • No

@Gui-FernandesBR Gui-FernandesBR added the Enhancement New feature or request, including adjustments in current codes label Nov 20, 2023
@Gui-FernandesBR Gui-FernandesBR added this to the Release v1.X.0 milestone Nov 20, 2023
@Gui-FernandesBR Gui-FernandesBR self-assigned this Nov 20, 2023
@codecov
Copy link

codecov bot commented Nov 20, 2023 •
edited
Loading

Codecov Report

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

Comparison is base (7eb4a73) 71.16% compared to head (181c0cf) 71.30%.
Report is 1 commits behind head on develop.

Files Patch % Lines
rocketpy/simulation/flight_data_importer.py 83.33% 15 Missing ⚠️
rocketpy/units.py 14.28% 6 Missing ⚠️
Additional details and impacted files 
@@             Coverage Diff             @@
##           develop     #474      +/-   ##
===========================================
+ Coverage    71.16%   71.30%   +0.14%     
===========================================
  Files           55       56       +1     
  Lines         9279     9370      +91     
===========================================
+ Hits          6603     6681      +78     
- Misses        2676     2689      +13
Flag Coverage Δ
unittests 71.30% <79.20%> (+0.14%) ⬆️

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
Copy link
Member Author

Gui-FernandesBR commented Nov 20, 2023

To see an example of the new class being used, see this branch: https://github.com/RocketPy-Team/RocketPy/tree/enh/add-juno3-example

giovaniceotto
giovaniceotto requested changes Nov 21, 2023
View reviewed changes
Copy link
Member

@giovaniceotto giovaniceotto left a comment

Choose a reason for hiding this comment

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

Great addition @Gui-FernandesBR! Can't wait to start using this more!

In fact, I hope we get a similar version that works directly with CSV files and other file formats common for rocket data loggers. I'll reach out to the community to see what kind of files we should support in a more long term scope.

I was pretty close to approving this PR as is, however, I believe I can help improve the implementation just a bit with two comments:

  1. Prefer a more explicit file name: data_frame_to_flight.py
  2. Avoid importing pandas, since it is a big module and will slow down importing this module. When you analyze the code, you really don't need pandas at all, even though you are working with pandas directly (Python is weird...)

@Gui-FernandesBR Gui-FernandesBR changed the title ENH: create DataFrameToFlight class ENH: create FlightDataImporter class Nov 26, 2023
@Gui-FernandesBR
Copy link
Member Author

Gui-FernandesBR commented Nov 26, 2023

For future reference...

Something that is getting into my nerves is that sometimes the flight data is separated into different files. This may cause differences in the time steps and others.

I guess that an easy solution would be converting the current private method __create_attributes to a public one, so the user could call it multiple times if needed. I think that could be a brilliant solution, but I'm not able to test it immediately so saving this for later.

Example:

image

@Gui-FernandesBR
Copy link
Member Author

Gui-FernandesBR commented Nov 28, 2023

@MateusStano , I'd like to have a discussion on the architecture of this class and the future others that will interact with this.

Let's please find a time to review this together.

MateusStano
MateusStano reviewed Dec 2, 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.

So overall it seems the user needs two things to use this class:

1 - A file with data in a valid format
2 - A dictionary mapping his data to flight values

I would say that makes a lot of sense, we just need a lot of documentation to support these requirements

@MateusStano
Copy link
Member

MateusStano commented Dec 2, 2023

Also, missing docs on all methods here, but I understand if you want to do that later

@Gui-FernandesBR Gui-FernandesBR changed the base branch from enh/built-in-compare to develop December 16, 2023 21:21
@Gui-FernandesBR Gui-FernandesBR requested a review from a team as a code owner December 16, 2023 21:21
@Gui-FernandesBR
Copy link
Member Author

Gui-FernandesBR commented Dec 17, 2023

@RocketPy-Team/code-owners I believe we are ready for a review now.
I'm still working on supplying an extended example on how to use the class and create good comparisons, but the FlighDataImporter itself is ready, I believe so.

@Gui-FernandesBR
Copy link
Member Author

Gui-FernandesBR commented Dec 17, 2023

Please take a look at #513 to see an example of this class being used.

MateusStano
MateusStano approved these changes Dec 18, 2023
View reviewed changes
Copy link
Member

@MateusStano MateusStano left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I think most comments I made are regarding documentation, so I am approving this one.

But unless you are planning to add proper documentation soon, I do not feel comfortable merging this to develop just yet

@Gui-FernandesBR
Copy link
Member Author

Gui-FernandesBR commented Dec 18, 2023

I think most comments I made are regarding documentation, so I am approving this one.

But unless you are planning to add proper documentation soon, I do not feel comfortable merging this to develop just yet

What do you mean by "proper documentation"?

MateusStano
MateusStano reviewed Dec 18, 2023
View reviewed changes
@MateusStano
Copy link
Member

MateusStano commented Dec 18, 2023

What do you mean by "proper documentation"?

My bad, I meant to delete that srry

@Gui-FernandesBR Gui-FernandesBR merged commit c740c38 into develop Dec 18, 2023
@Gui-FernandesBR Gui-FernandesBR deleted the enh/compare-flight-data branch December 18, 2023 03:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@MateusStano MateusStano MateusStano approved these changes

@phmbressan phmbressan Awaiting requested review from phmbressan

@giovaniceotto giovaniceotto Awaiting requested review from giovaniceotto

Assignees

@Gui-FernandesBR Gui-FernandesBR

Labels

Enhancement New feature or request, including adjustments in current codes

Projects

No open projects
LibDev Roadmap
Status: Closed

Milestone

Release v1.X.0

Development

Successfully merging this pull request may close these issues.

4 participants

@Gui-FernandesBR @MateusStano @giovaniceotto @lint-action
X Tutup