Refactor graph creators #219
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Better separation of concerns between computations and plotting logic, and switch to composition instead of inheritance for the graph creators. Also added better type safety using Protocols, better abstract base classes and data transfer objects.
Reviewer's GuideThis PR refactors the GraphCreator framework into a composition-based architecture, moving computation and plotting logic into dedicated modules, introducing base models and helpers for uniform interfaces, and simplifying the GraphCreator API and package structure. Sequence Diagram: GraphCreator.create_graph Core Interaction FlowsequenceDiagram
participant Client
participant GCreator as ConcreteGraphCreator
participant GComputation as ConcreteComputation
participant GPlotter as ConcretePlotter
Client->>GCreator: create_graph(measurements_manager)
activate GCreator
GCreator->>GCreator: _create_computation(measurements_manager)
activate GCreator
GCreator-->>GComputation: new(measurements, params)
deactivate GCreator
GCreator->>GComputation: compute()
activate GComputation
GComputation-->>GCreator: computation_result
deactivate GComputation
GCreator->>GPlotter: plot(computation_result)
activate GPlotter
GPlotter-->>GCreator: figure
deactivate GPlotter
GCreator->>GCreator: _save_figure(figure)
deactivate GCreator
Class Diagram: GraphCreator Compositional ArchitectureclassDiagram
class GraphCreator {
<<Abstract>>
#_config: ShakeTuneConfig
#_version: str
#_type: str
#_folder: Path
#_output_target: Optional~Path~
#_computation_class: Type~Computation~
#_plotter: PlotterStrategy
+register(graph_type: str)* decorator
+__init__(config: ShakeTuneConfig, computation_class: Type~Computation~, plotter_class: Type~PlotterStrategy~)
+configure(**kwargs)* void
#_create_computation(measurements_manager: MeasurementsManager)* Computation
+create_graph(measurements_manager: MeasurementsManager) void
#_save_figure(fig: Figure) void
}
class Computation {
<<Interface>>
+compute()* ComputationResult
}
class PlotterStrategy {
<<Abstract>>
+KLIPPAIN_COLORS: dict
#_logo_image: any
+__init__()
+plot(data: ComputationResult)* Figure
+add_logo(fig: Figure, position: List~float~) void
+add_version_text(fig: Figure, version: str, position: tuple) void
+add_title(fig: Figure, title_lines: List~dict~) void
}
class ComputationResult {
<<Abstract>>
+metadata: GraphMetadata
+measurements: List~Measurement~
+get_plot_data()* Dict~str, any~
}
GraphCreator *-- "1" PlotterStrategy : _plotter (instance)
GraphCreator --> "1" Computation : _computation_class (Type)
GraphCreator ..> Computation : creates instance via _create_computation()
class VibrationsGraphCreator {
-_kinematics: Optional~str~
-_accel: Optional~float~
-_motors: Optional~List~any~~
+__init__(config: ShakeTuneConfig)
+configure(kinematics: str, accel: Optional~float~, motors: Optional~List~any~~) void
#_create_computation(measurements_manager: MeasurementsManager) VibrationsComputation
}
VibrationsGraphCreator --|> GraphCreator
class VibrationsComputation {
+__init__(measurements, kinematics, accel, max_freq, motors, st_version)
+compute() VibrationsResult
}
VibrationsComputation ..|> Computation
class VibrationsPlotter {
+plot(result: VibrationsResult) Figure
}
VibrationsPlotter --|> PlotterStrategy
class VibrationsResult {
+get_plot_data() Dict~str, any~
}
VibrationsResult --|> ComputationResult
VibrationsGraphCreator ..> VibrationsComputation : creates
VibrationsComputation ..> VibrationsResult : creates & returns
VibrationsPlotter ..> VibrationsResult : uses in plot()
Class Diagram: GraphCreatorFactory UpdateclassDiagram
class GraphCreatorFactory {
<<Factory>>
+create_graph_creator(graph_type: str, config: ShakeTuneConfig)* GraphCreator
}
class GraphCreator {
<<Abstract>>
registry: dict~str, Type[GraphCreator]~
graph_type: str
+register(graph_type: str)* decorator
}
GraphCreatorFactory ..> GraphCreator : uses registry
Class Diagram: New Data Models (GraphMetadata and its use in ComputationResult)classDiagram
class GraphMetadata {
+title: str
+subtitle: Optional~str~
+version: str
+timestamp: Optional~str~
+additional_info: Dict~str, Any~
}
class ComputationResult {
<<Abstract>>
+metadata: GraphMetadata
+measurements: List~Measurement~
}
ComputationResult o-- "1" GraphMetadata : contains
Class Diagram: New PlottingUtils ComponentsclassDiagram
namespace shaketune.graph_creators.plotting_utils {
class AxesConfiguration {
+configure_axes(ax, title, xlabel, ylabel, legend, grid, sci_axes) FontProperties
}
class PeakAnnotator {
+annotate_peaks(ax, freqs, data, peaks, threshold, color, label_prefix) void
}
class PlottingConstants {
+KLIPPAIN_COLORS : dict
}
class SpectrogramHelper {
+plot_spectrogram(ax, t, bins, pdata, max_freq, title) void
}
class TableHelper {
+add_table(ax, cell_text, row_labels, col_labels, title) void
}
}
class PlotterStrategy {
#_plotter: PlotterStrategy
}
PlotterStrategy --o plotting_utils.PlottingConstants : uses
PlotterStrategy --o plotting_utils.AxesConfiguration : uses
PlotterStrategy --o plotting_utils.SpectrogramHelper : uses
PlotterStrategy --o plotting_utils.TableHelper : uses
PlotterStrategy --o plotting_utils.PeakAnnotator : uses
Class Diagram: MeasurementsManager Method UpdatesclassDiagram
class MeasurementsManager {
+measurements: List~Measurement~
+load_from_stdata(filename: Path) List~Measurement~
+load_from_csvs(klipper_CSVs: List~Path~) List~Measurement~
+add_measurement(name: str, samples: List~tuple~) void
}
note for MeasurementsManager "load_from_stdata now returns List[Measurement].
load_from_csvs updated for CLI support."
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Closed
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @Frix-x - I've reviewed your changes - here's some feedback:
- The reliance on the SHAKETUNE_IN_CLI environment variable in MeasurementsManager.load_from_csvs is brittle—consider passing an explicit CLI-mode flag or strategy into the loader rather than branching on os.environ directly.
- You still call get_shaper_calibrate_module() in every computation class—consider injecting the calibrator (e.g. via the GraphCreator constructor or a shared service) to avoid duplication and make the modules easier to test.
Here's what I looked at during the review
- 🟡 General issues: 5 issues found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 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.
this will allow to manually specify a specific chip for the measurement and override the default behavior where S&T usually try to find the best suited chip based on what is found in `[resonance_tester]` config section
This was referenced May 25, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary by Sourcery
Refactor graph creators to adopt a composition-based architecture by separating computation and plotting into dedicated modules, simplify the GraphCreator API, and add command-line interface support and documentation.
New Features:
Enhancements:
computations/andplotters/packages and introduce shared base models and utilities.create_graphworkflow.plotting_utilsandbase_models.Documentation:
docs/cli_usage.mdand update README to reference CLI.