ENH: Barometer, ScalarSensors and InertialSensors #608
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## enh/sensors-impl #608 +/- ##
====================================================
- Coverage 73.78% 73.72% -0.07%
====================================================
Files 64 62 -2
Lines 10049 10064 +15
====================================================
+ Hits 7415 7420 +5
- Misses 2634 2644 +10 ☔ View full report in Codecov by Sentry. |
Gui-FernandesBR
left a comment
Gui-FernandesBR
left a comment
There was a problem hiding this comment.
@MateusStano could you pylint your code and fix some warning messages?
There is the make pylint command.
phmbressan
left a comment
phmbressan
left a comment
There was a problem hiding this comment.
Good work. I agree with your division of Sensor into Inertial and Scalar since they seem to have different contracts parameter-wise.
Before approving, I believe some of the comments on this PR should be addressed, specially the singular class naming one.
|
@Gui-FernandesBR @phmbressan comments addressed, files renamed and most of pylint complaint fixed. |
Gui-FernandesBR
left a comment
Gui-FernandesBR
left a comment
There was a problem hiding this comment.
Good progress!
Please make sure to create a task list for us to follow your progress on the sensors implementation. I just don't know how much of work is still needed.
rocketpy/sensors/accelerometer.py
Outdated
| export_sensors_measured_data( | ||
| sensor=self, | ||
| filename=filename, | ||
| file_format=file_format, | ||
| data_labels=("t", "ax", "ay", "az"), | ||
| ) |
There was a problem hiding this comment.
Once again.
If filename="this_is_my_file.csv", it is redundant asking what is the file_format. You can (almost) automatically guess it.
There was a problem hiding this comment.
I don't know about this. The file_format I guess refers more to the way data is saved in the file. Someone could want a .txt file with the data written in the csv format or in the json format
For me, it seems simpler just to have this choice as an argument, but I understand it can be redundant information in certain cases
There was a problem hiding this comment.
Someone could want a .txt file with the data written in the csv format or in the json format
This is not reasonable to me. I don't see why someone would try to do this.
I think it's more annoying having to write "filename.txt, file_format='xt'", I don't see why this is simpler to you.
Gui-FernandesBR
left a comment
Gui-FernandesBR
left a comment
There was a problem hiding this comment.
You can merge after solving the tests.
Good work.
Please list the remaining tasks for the future PRs. I don't know how much is still left to be done.
Pull request type
Checklist
black rocketpy/ tests/) has passed locallypytest tests -m slow --runslow) have passed locallyNew
I have added the
Barometerclass. Since this sensor does not have axes, the class structure of the sensors had to change. The old abstractSensorsclass still exists, and it inherits to:InertialSensors: Deals with vector measurements. Is inherited byAccelerometerandGyroscopeScalarSensors: Deals with scalar measurements. Is inherited only byBarometerfor nowThe
sensor_testing.ipynbnotebook has a working barometer