feat: Mode selection update#162
Conversation
|
To be merged after #160 as this directly follows those changes. |
f7e3df2 to
1085380
Compare
…-selection-update Resolved conflicts by accepting incoming changes: - examples/modeselect.ipynb - src/few/amplitude/ampinterp2d.py - src/few/amplitude/romannet.py - src/few/utils/modeselector.py - tests/test_mode_selector.py
lorenzsp
left a comment
There was a problem hiding this comment.
Final Assessment: Mode Selection Update Branch Review
Based on my comprehensive analysis of the mode-selection-update branch, here's my assessment of whether it's ready to merge into master:
✅ READY TO MERGE - with minor cleanup recommended
Summary of Changes
The branch contains a significant update to the mode selection functionality with 30+ commits adding 742 lines and removing 1,013 lines across 18 files. Key improvements include:
Code Quality Assessment
Architecture: Well-structured refactoring that moves common amplitude functionality to a base class
New Features: Enhanced mode selector with better negative m-mode handling and improved user interface
Documentation: Comprehensive docstrings and updated tutorial examples
Error Handling: Robust input validation and meaningful error messages
Testing Status ✅
All tests passing: Mode selector tests and full test suite run successfully
Clean API design with backward compatibility
Proper handling of edge cases (negative m-modes, duplicate modes)
Performance optimizations and user warnings for large mode selections
Good separation of concerns between amplitude generation and mode selection
Merge Readiness Checklist
✅ No merge conflicts with master
✅ All tests passing
✅ Code follows project conventions
✅ Documentation updated appropriately
✅ Backward compatibility maintained
✅ New functionality properly tested
This PR overhauls the mode selection framework in FEW used to identify dominant waveform modes in order to speed up the waveform generation. The new implementation better estimates the importance of each mode, particularly when noise-weighting, and is more efficient in cases when a pre-determined list of modes is required.
The
few.utils.modeselector.ModeSelectorclass now takes amplitude and ylm modules as input, and handles the generation of mode amplitudes. This has two main benefits:Rather than sorting the mode power at each trajectory step, the SNR of each mode is approximated by folding in the duration of each spline segment, under the assumption of mode orthogonality (which is not quite true for higher-order l modes, but is still a reasonable approximation). This both reduces the cost of mode selection (as now only one set of values must be sorted, rather than 50-100 as before) and is far more accurate. The previous method assigned too high an importance to modes that only appeared in the final moments of inspiral, but contribute negligibly to the waveform SNR. With this change, the
mode_selection_thresholdparameter now closely matches the mismatch in the waveform obtained, which is significantly more intuitive for the end user and performs more consistently over the parameter space.Finally, this also includes a utility
few.utils.modeselector.get_selected_modes_from_initial_conditionsthat takes trajectory and mode selection modules as well as initial conditions and returns the sorted modes, which is useful for directly examining the mode content as a function of parameter space in one function call.📚 Documentation preview 📚: https://fastemriwaveforms--162.org.readthedocs.build/en/162/