-
-
Notifications
You must be signed in to change notification settings - Fork 87
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
Refactoring of the graph creators #171
Conversation
Reviewer's Guide by SourceryThis pull request implements a major refactoring of the graph creators in the Shake&Tune project, focusing on three main aspects: introducing a Factory pattern for graph creators, separating computation from plotting logic, and improving code organization. The changes make the codebase more maintainable and extensible while reducing code duplication. Class diagram for GraphCreator and its subclassesclassDiagram
class GraphCreator {
+ShakeTuneConfig config
+String graph_date
+String version
+String type
+Path folder
+Plotter plotter
+String custom_filepath
+void _save_figure(Figure fig, MeasurementsManager measurements_manager, Optional~String~ axis_label)
+String get_type()
+void override_output_target(String filepath)
+void create_graph(MeasurementsManager measurements_manager)
+void clean_old_files(int keep_results)
}
class VibrationsGraphCreator {
+Optional~String~ kinematics
+Optional~float~ accel
+Optional~List~MotorsConfigParser~ motors
+void configure(String kinematics, float accel, MotorsConfigParser motor_config_parser)
+void create_graph(MeasurementsManager measurements_manager)
}
class ShaperGraphCreator {
+Optional~float~ max_smoothing
+Optional~float~ scv
+Optional~float~ accel_per_hz
+void configure(float max_smoothing, float scv, float accel_per_hz)
+void create_graph(MeasurementsManager measurements_manager)
}
class StaticGraphCreator {
+Optional~float~ freq
+Optional~float~ duration
+Optional~float~ accel_per_hz
+void configure(float freq, float duration, float accel_per_hz)
+void create_graph(MeasurementsManager measurements_manager)
}
GraphCreator <|-- VibrationsGraphCreator
GraphCreator <|-- ShaperGraphCreator
GraphCreator <|-- StaticGraphCreator
GraphCreator : +registry
GraphCreator : +register(String graph_type)
GraphCreator : +void _save_figure(Figure fig, MeasurementsManager measurements_manager, Optional~String~ axis_label)
GraphCreator : +String get_type()
GraphCreator : +void override_output_target(String filepath)
GraphCreator : +void create_graph(MeasurementsManager measurements_manager)
GraphCreator : +void clean_old_files(int keep_results)
Class diagram for VibrationGraphComputation and related classesclassDiagram
class VibrationGraphComputation {
+List~Measurement~ measurements
+String kinematics
+float accel
+float max_freq
+String st_version
+List~String~ motors
+compute()
+_compute_motor_profiles()
+_compute_dir_speed_spectrogram()
+_compute_angle_powers()
+_compute_speed_powers()
+_filter_and_split_ranges()
+_compute_symmetry_analysis()
+_extract_angle_and_speed()
}
class Measurement {
+String name
+List~float~ samples
}
VibrationGraphComputation o-- Measurement
VibrationGraphComputation : +List~Measurement~ measurements
VibrationGraphComputation : +String kinematics
VibrationGraphComputation : +float accel
VibrationGraphComputation : +float max_freq
VibrationGraphComputation : +String st_version
VibrationGraphComputation : +List~String~ motors
VibrationGraphComputation : +compute()
VibrationGraphComputation : +_compute_motor_profiles()
VibrationGraphComputation : +_compute_dir_speed_spectrogram()
VibrationGraphComputation : +_compute_angle_powers()
VibrationGraphComputation : +_compute_speed_powers()
VibrationGraphComputation : +_filter_and_split_ranges()
VibrationGraphComputation : +_compute_symmetry_analysis()
VibrationGraphComputation : +_extract_angle_and_speed()
Class diagram for ShaperGraphComputation and related classesclassDiagram
class ShaperGraphComputation {
+List~Measurement~ measurements
+Optional~float~ accel_per_hz
+float scv
+Optional~float~ max_smoothing
+float max_freq
+String st_version
+compute()
+_calibrate_shaper()
}
class Measurement {
+String name
+List~float~ samples
}
ShaperGraphComputation o-- Measurement
ShaperGraphComputation : +List~Measurement~ measurements
ShaperGraphComputation : +Optional~float~ accel_per_hz
ShaperGraphComputation : +float scv
ShaperGraphComputation : +Optional~float~ max_smoothing
ShaperGraphComputation : +float max_freq
ShaperGraphComputation : +String st_version
ShaperGraphComputation : +compute()
ShaperGraphComputation : +_calibrate_shaper()
Class diagram for StaticGraphComputation and related classesclassDiagram
class StaticGraphComputation {
+List~Measurement~ measurements
+float freq
+float duration
+float max_freq
+Optional~float~ accel_per_hz
+String st_version
+compute()
}
class Measurement {
+String name
+List~float~ samples
}
StaticGraphComputation o-- Measurement
StaticGraphComputation : +List~Measurement~ measurements
StaticGraphComputation : +float freq
StaticGraphComputation : +float duration
StaticGraphComputation : +float max_freq
StaticGraphComputation : +Optional~float~ accel_per_hz
StaticGraphComputation : +String st_version
StaticGraphComputation : +compute()
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @Frix-x - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider breaking down some of the longer plotting methods in the Plotter class in a future PR to improve readability and maintainability
Here's what I looked at during the review
- 🟡 General issues: 2 issues found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟡 Complexity: 1 issue found
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com>
Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com>
Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com>
This is a big code change in order to: