refactor: comprehensive code review fixes and optimizations#44
Open
vudxlab wants to merge 3 commits intodagghe:mainfrom
Open
refactor: comprehensive code review fixes and optimizations#44vudxlab wants to merge 3 commits intodagghe:mainfrom
vudxlab wants to merge 3 commits intodagghe:mainfrom
Conversation
This commit addresses multiple code quality issues, performance bottlenecks, and potential bugs identified during a comprehensive codebase review: HIGH PRIORITY FIXES: - Remove debug print statements from ssi.py (lines 138, 141) left in production - Fix variable shadowing in ssi.py line 147 (variable 'j' used in nested loops) - Optimize O(n²) array building in ssi.py - replace inefficient np.vstack in loop with pre-allocated arrays for better performance - Replace ambiguous variable name 'l' with 'num_channels' throughout ssi.py for improved readability and maintainability MEDIUM PRIORITY FIXES: - Add warning comments for global np.seterr() in ssi.py and plscf.py to document potential issues with global error suppression - Improve exception handling in gen.py - replace overly broad Exception catches with specific exceptions (ValueError, ZeroDivisionError, np.linalg.LinAlgError) and add debug logging - Fix comparison pattern in plscf.py - replace 'check.any() == False' with cleaner 'not check.any()' syntax LOW PRIORITY FIXES: - Remove unnecessary self-assignments in setup/single.py - Convert inline comments to proper documentation format - Remove commented debug statement from clus.py - Fix typo in comment: 'channnels' -> 'channels' These changes improve code quality, performance, and maintainability without altering the external API or functionality.
Add new `interactive=True` parameter to plot_stab() method in SSI and pLSCF algorithms, enabling interactive pole selection with spectrum overlay in a single unified interface. Key features: - Interactive GUI for pole selection using Tkinter - Real-time spectrum (CMIF) overlay on secondary axis - Toggle spectrum visibility during picking session - Menu controls for unstable poles and spectrum - Consistent API across SSI and pLSCF algorithms Changes: - src/pyoma2/support/sel_from_plot.py: * Add spectrum and nSv parameters to __init__ * Add spectrum overlay support in plot_stab method * Add toggle_spectrum() method for runtime control * Add "Spectrum Overlay" menu for interactive toggling * Update class docstrings with new attributes - src/pyoma2/algorithms/ssi.py: * Add interactive parameter to plot_stab() * Integrate SelFromPlot for interactive mode * Update docstrings with interactive controls - src/pyoma2/algorithms/plscf.py: * Add spectrum, nSv, and interactive parameters * Add spectrum overlay support (new feature) * Integrate SelFromPlot for interactive mode * Update docstrings with full parameter list - Examples/interactive_stability_chart_demo.py: * Comprehensive demo script showing new features * Examples for SSI and pLSCF algorithms * API comparison (old vs new) - INTERACTIVE_STABILITY_CHART.md: * Complete Vietnamese documentation * Usage examples and API reference * Interactive controls guide * Troubleshooting section API usage: # Old way (2 separate methods): ssicov.plot_stab(spectrum=True) # View only ssicov.mpe_from_plot() # Interactive, no spectrum # New way (unified): ssicov.plot_stab(spectrum=True, interactive=True) Interactive controls: - SHIFT + LEFT mouse: Select pole - SHIFT + RIGHT mouse: Remove last pole - SHIFT + MIDDLE mouse: Remove closest pole - Menu > Spectrum Overlay: Toggle spectrum - Menu > Show/Hide Unstable Poles: Toggle poles Benefits: - Unified workflow: single method instead of two - Spectrum helps identify physical modes - Real-time spectrum toggling during picking - Backward compatible: default interactive=False - Consistent interface across algorithms Resolves: Interactive stability chart with spectrum overlay request
…-LD22E Add interactive mode picking to stability chart
Collaborator
|
Hi @vudxlab thank you for yuor PR. Tests are failing, you can try to run them locally on your changes and update this PR |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
This commit addresses multiple code quality issues, performance bottlenecks, and potential bugs identified during a comprehensive codebase review:
HIGH PRIORITY FIXES:
MEDIUM PRIORITY FIXES:
LOW PRIORITY FIXES:
These changes improve code quality, performance, and maintainability without altering the external API or functionality.