Consolidate Q binning options into drop down menu#185
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## next #185 +/- ##
==========================================
+ Coverage 63.60% 64.75% +1.15%
==========================================
Files 34 36 +2
Lines 6761 6804 +43
==========================================
+ Hits 4300 4406 +106
+ Misses 2461 2398 -63 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
📝 WalkthroughWalkthroughThe changes remove the global final rebin checkbox and its associated configuration flag from the QuickNXS application, consolidating final rebin control to per-run settings. The global final rebin checkbox widget is replaced by a push button that, when pressed, propagates the global Q-step value to all runs, enabling per-run final rebin accordingly. The backend logic is updated to remove tracking of the global final rebin flag and instead programmatically set per-run configurations based on the global Q-step value. UI signal-slot connections are updated to reflect these changes. New UI tests verify synchronization between UI elements and configuration, reflectivity recalculation triggers, and consistent behavior across multiple data tabs. Related configuration class attributes and data handling logic are simplified by removing global final rebin options. Sequence Diagram(s)sequenceDiagram
participant User
participant MainWindow
participant MainHandler
participant ReductionTable
User->>MainWindow: Press propagate_final_rebin_button
MainWindow->>MainHandler: propagate_final_rebin_to_run_config()
MainHandler->>MainHandler: For each run in active tab:
MainHandler->>MainHandler: Set do_final_rebin_run = True
MainHandler->>MainHandler: Set final_rebin_step_run = global Q-step value
MainHandler->>ReductionTable: Update reduction table rows
MainHandler->>MainWindow: Update run-level UI elements (checkbox, spinbox)
MainWindow->>MainWindow: Trigger reflectivity recalculation and replotting
sequenceDiagram
participant User
participant MainWindow
User->>MainWindow: Toggle per-run final_rebin_checkbox_run or edit q_rebin_spinbox_run
MainWindow->>MainWindow: changeRegionValues()
MainWindow->>MainWindow: Update per-run configuration and UI
MainWindow->>MainWindow: Recalculate and replot reflectivity for current run
sequenceDiagram
participant TestSuite
participant MainWindow
TestSuite->>MainWindow: Simulate UI interactions (checkbox toggles, button press, spinbox edits)
MainWindow->>MainWindow: Update configuration and UI accordingly
MainWindow->>TestSuite: Verify reduction table values and checkbox states
MainWindow->>TestSuite: Verify reflectivity recalculation triggered appropriately
TestSuite->>MainWindow: Switch between data tabs
MainWindow->>TestSuite: Verify consistent final rebin state per tab
Estimated code review effort3 (~45 minutes) ✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/quicknxs/interfaces/event_handlers/configuration_handler.py (1)
59-61: Consider enhancing method documentation.The new method correctly handles the mutual exclusivity logic by delegating to the main window.
def on_global_final_rebin_toggled(self, state: int): - """Handle the mutually exclusive global and per-run final rebin configuration""" + """Handle the mutually exclusive global and per-run final rebin configuration. + + Parameters + ---------- + state : int + The checkbox state (Qt.Checked, Qt.Unchecked, or Qt.PartiallyChecked) + """ self.main_window.toggleFinalRebinGlobal(state)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/quicknxs/interfaces/event_handlers/configuration_handler.py(5 hunks)src/quicknxs/interfaces/event_handlers/main_handler.py(1 hunks)src/quicknxs/interfaces/main_window.py(2 hunks)src/quicknxs/ui/ui_main_window.ui(1 hunks)test/ui/test_final_rebin.py(1 hunks)test/ui/test_reflectivity_extraction_global.py(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (4)
test/ui/test_reflectivity_extraction_global.py (1)
test/ui/ui_utilities.py (1)
setValue(21-26)
src/quicknxs/interfaces/event_handlers/configuration_handler.py (1)
src/quicknxs/interfaces/main_window.py (1)
toggleFinalRebinGlobal(591-593)
src/quicknxs/interfaces/main_window.py (1)
src/quicknxs/interfaces/event_handlers/main_handler.py (1)
check_region_values_changed(1275-1372)
test/ui/test_final_rebin.py (2)
src/quicknxs/interfaces/main_window.py (3)
reduction_cell_activated(317-322)addDataTable(438-453)current_table_changed(495-502)test/ui/ui_utilities.py (3)
setText(12-18)set_current_file_by_run_number(71-74)setValue(21-26)
🔇 Additional comments (19)
src/quicknxs/ui/ui_main_window.ui (1)
6534-6549: No issues – connection looks correctThe new signal-slot entry cleanly hooks
q_rebin_spinbox_run.editingFinished()toMainWindow.changeRegionValues().
No duplicate connections found, and it matches the pattern used for similar spin-boxes elsewhere. 👍test/ui/test_reflectivity_extraction_global.py (2)
125-135: LGTM! Clear test scope specification.The comments now clearly distinguish between global and run-specific widget testing, improving test readability and maintainability.
137-147: Excellent addition of run-specific widget tests.These tests correctly validate that:
- Toggling
final_rebin_checkbox_runtriggers reflectivity recalculation only for the current run (1 call)- Editing
q_rebin_spinbox_runtriggers reflectivity recalculation only for the current run (1 call)This properly verifies the mutual exclusivity behavior between global and per-run rebin configurations.
src/quicknxs/interfaces/event_handlers/configuration_handler.py (4)
3-4: Good migration to qtpy for cross-Qt compatibility.Using
qtpyinstead of direct PyQt5 imports provides better compatibility across different Qt versions.
44-44: Correct Qt reference update for qtpy.The Qt constant reference is properly updated to work with qtpy imports.
88-89: Excellent configuration key naming update.Adding the "_global" suffix clearly distinguishes global settings from per-run settings, improving code clarity and preventing configuration conflicts.
125-127: Proper signal connection for mutual exclusivity.The signal connection correctly links the global final rebin checkbox to the new handler, ensuring the mutual exclusivity logic is triggered when the checkbox state changes.
src/quicknxs/interfaces/main_window.py (4)
262-262: Good enhancement with optional parameter.Adding the optional
change_typeparameter provides better control over when reflectivity recalculation occurs while maintaining backward compatibility.
268-273: Clear and comprehensive parameter documentation.The documentation clearly explains the purpose and values of the
change_typeparameter, making the method's behavior explicit.
277-278: Smart fallback logic for backward compatibility.The conditional logic properly determines the change type when not explicitly provided, ensuring existing callers continue to work correctly.
589-589: Explicit change type ensures reflectivity update.Passing
change_type=1explicitly ensures that toggling the final rebin run state triggers reflectivity recalculation, which aligns with the PR objectives for proper UI synchronization.src/quicknxs/interfaces/event_handlers/main_handler.py (2)
1776-1780: LGTM: Clear documentation of mutual exclusivity behavior.The updated docstring clearly explains the comprehensive nature of the mutual exclusivity enforcement across all data tabs and runs.
1786-1801: No cross-section name mismatch—assumption holds.All
NexusData.cross_sectionsmappings use the same set of keys (loaded from the file) across runs, andDataManager.active_cross_section.nameis guaranteed to be one of those keys for everyNexusDatainpeak_reduction_lists(tests cover these cases extensively). No per‐run variation in cross‐section names occurs.test/ui/test_final_rebin.py (6)
8-32: Well-structured test helper function.The helper function properly sets up test data with both direct beam and reduction table entries, and allows configuration of global rebin state. Good use of the ui_utilities module for consistent widget interaction.
34-51: Comprehensive test of run rebin table synchronization.The test correctly verifies both enabling and disabling scenarios, checking both the table content and editability flags. Good coverage of the mutual exclusivity behavior.
53-75: Excellent coverage of global rebin mutual exclusivity.This test thoroughly verifies the key behavior changes:
- Global rebin disables run rebin checkbox
- Table Q-step values are cleared
- Table cells become non-editable
- Disabling global rebin restores editability without auto-enabling run rebin
The assertions correctly check both UI state and editability flags.
77-106: Good bidirectional synchronization test.The test properly verifies that changes flow both ways between the spinbox and reduction table. The use of ui_utilities for consistent widget interaction is appropriate.
108-133: Effective parameterized test for replotting behavior.The parameterized approach efficiently tests both global and run-specific Q-step spinboxes. The mocking strategy correctly isolates the plotting behavior and verifies the trigger mechanism.
135-165: Comprehensive cross-tab configuration persistence test.This test effectively verifies that configuration changes propagate across data tabs and are preserved when switching between tabs. The scenario properly tests the complex interaction between multiple tabs and global vs. run-specific settings.
backmari
changed the title
backmari
force-pushed
the
final_rebin_signals
branch
from
bcae198 to
e3b0178
Compare
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/quicknxs/interfaces/event_handlers/main_handler.py (1)
1759-1790: Well-implemented propagation logic with proper signal management.The method correctly updates both internal configuration and UI state while blocking signals to prevent unintended cascading effects. The docstring clearly documents the caller's responsibility for triggering recalculation.
Consider enhancing testability by accepting the Q-step value as an optional parameter:
- def propagate_final_rebin_to_run_config(self): + def propagate_final_rebin_to_run_config(self, q_step: Optional[float] = None): """Enable final rebin with the given Q-step for all runs in the active data tab. Note: This function updates the UI and internal configuration state while blocking all signals. The caller is responsible for triggering recalculation and replotting. """ - q_step = self.ui.q_rebin_spinbox_global.value() + if q_step is None: + q_step = self.ui.q_rebin_spinbox_global.value()This would make the method easier to test without requiring UI setup.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
src/quicknxs/interfaces/configuration.py(1 hunks)src/quicknxs/interfaces/data_handling/data_set.py(2 hunks)src/quicknxs/interfaces/event_handlers/configuration_handler.py(3 hunks)src/quicknxs/interfaces/event_handlers/main_handler.py(2 hunks)src/quicknxs/interfaces/main_window.py(1 hunks)src/quicknxs/ui/ui_main_window.ui(4 hunks)test/ui/test_final_rebin.py(1 hunks)test/ui/test_main_window.py(2 hunks)test/ui/test_reflectivity_extraction_global.py(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (4)
test/ui/test_reflectivity_extraction_global.py (1)
test/ui/ui_utilities.py (1)
setValue(21-26)
src/quicknxs/interfaces/main_window.py (1)
src/quicknxs/interfaces/event_handlers/main_handler.py (1)
propagate_final_rebin_to_run_config(1759-1789)
test/ui/test_final_rebin.py (2)
src/quicknxs/interfaces/main_window.py (2)
reduction_cell_activated(310-315)addDataTable(431-446)test/ui/ui_utilities.py (3)
setText(12-18)set_current_file_by_run_number(71-74)setValue(21-26)
src/quicknxs/interfaces/event_handlers/main_handler.py (2)
src/quicknxs/interfaces/data_manager.py (3)
reduction_list(94-96)reduction_list(99-100)update_configuration(485-495)src/quicknxs/interfaces/data_handling/data_set.py (2)
update_configuration(507-519)update_configuration(934-940)
🔇 Additional comments (12)
src/quicknxs/interfaces/configuration.py (1)
38-39: LGTM: Comment updated to reflect single optionThe comment change from "options" to "option" is appropriate since the global final rebin boolean flag
do_final_rebin_globalhas been removed, leaving only thefinal_rebin_step_globalconfiguration.src/quicknxs/interfaces/data_handling/data_set.py (1)
821-824: LGTM: Simplified final rebin logicThe direct use of per-run final rebin settings (
conf.do_final_rebin_run,conf.final_rebin_step_run) simplifies the data handling logic by removing the conditional choice between global and per-run configurations. This aligns with the new propagation model where the global settings are applied to per-run configurations via the UI button.Also applies to: 834-834
src/quicknxs/interfaces/event_handlers/configuration_handler.py (3)
3-4: LGTM: Modernized imports from PyQt5 to qtpyThe import change from PyQt5 to qtpy improves compatibility and follows modern PyQt practices.
44-44: LGTM: Updated Qt reference for qtpy compatibilityThe Qt reference has been correctly updated to match the qtpy import style.
84-84: LGTM: Configuration widget updates align with UI changesThe removal of
"final_rebin_checkbox_global"and the config key update from"final_rebin_step"to"final_rebin_step_global"correctly reflect the UI refactoring where the global checkbox was removed and the spinbox now specifically targets the global step configuration.test/ui/test_main_window.py (1)
329-329: LGTM: Improved configuration initialization orderMoving
Configuration.setup_default_values()beforeMainWindow()instantiation ensures that default configuration values are properly set before UI initialization, which is a good practice.Also applies to: 389-389
test/ui/test_reflectivity_extraction_global.py (2)
124-130: LGTM: Test validates new propagate button functionalityThe test correctly simulates the new workflow where editing the global Q-rebin spinbox and clicking the propagate button triggers reflectivity recalculation for all runs. This properly validates the new propagation mechanism.
132-142: LGTM: Tests validate per-run control behaviorThe new test cases correctly verify that per-run final rebin controls (both checkbox and spinbox) only trigger reflectivity recalculation for the current run, not all runs. This ensures the distinction between global and per-run operations is working correctly.
src/quicknxs/interfaces/main_window.py (1)
579-585: Clean implementation of the final rebin propagation.The method properly delegates configuration updates to the handler and triggers the necessary recalculation. The separation of concerns between UI and business logic is well maintained.
src/quicknxs/interfaces/event_handlers/main_handler.py (1)
861-863: Good improvement to Q-step formatting consistency.Using f-string with fixed 3 decimal places ensures consistent display format across the UI.
src/quicknxs/ui/ui_main_window.ui (2)
6539-6554: Good call adding spin-box → slot wiringHooking
editingFinished()ofq_rebin_spinbox_runtochangeRegionValues()ensures per-run Q-step edits immediately trigger replot/recalc.
6668-6682: Verify recalculation path when only the global Q-step value changesThe new connection fires when the button is pressed, but if a user edits
q_rebin_spinbox_globaland immediately presses “Reduce…” without pressing this button, the per-run tables might still hold stale Q-step values.Consider also connecting
editingFinished()ofq_rebin_spinbox_globalto the samepropagate_final_rebin_to_runs()slot (or disabling “Reduce…” until propagation occurred).
backmari
changed the title
…inning config only on run level
backmari
force-pushed
the
final_rebin_signals
branch
from
b5dff6e to
8d2ba5a
Compare
backmari
changed the title
There was a problem hiding this comment.
Pull Request Overview
This PR consolidates Q binning options in the QuickNXS interface by replacing separate global/run checkboxes with a unified dropdown menu approach. The changes improve UI clarity by making mutually exclusive binning options more obvious and consistent.
- Replaces separate global/run final rebin checkboxes with a
Binning Typedropdown selector - Consolidates Q binning configuration to be stored consistently at the run level
- Adds a convenience button to apply the same binning settings to all runs
Reviewed Changes
Copilot reviewed 15 out of 26 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/quicknxs/interfaces/configuration.py | Introduces BinningType enum and updates Configuration class to use new binning attributes |
| src/quicknxs/ui/binningtype_combobox.py | New custom QComboBox widget for binning type selection |
| src/quicknxs/ui/ui_main_window.ui | Updates UI layout to replace checkboxes with dropdown menus and buttons |
| src/quicknxs/interfaces/main_window.py | Removes old toggle methods and adds new propagate function |
| src/quicknxs/interfaces/event_handlers/main_handler.py | Updates configuration handling and table management for new binning approach |
| test/ui/test_final_rebin.py | New comprehensive test file for binning functionality |
| test/ui/test_binningtypeselection.py | Unit tests for the new combobox widget |
Comments suppressed due to low confidence (1)
test/ui/test_final_rebin.py:6
- The parameter name
final_rebin_enabledis misleading since 'final rebin' terminology has been replaced with 'binning options'. Consider renaming tobinning_enabledor similar.
def _populate_reduction_and_direct_beam_tables(main_window, final_rebin_enabled=False):
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Jose Borreguero <borreguero@gmail.com> Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
for more information, see https://pre-commit.ci
… DirectBeamTableColumn
Description of work:
The toggling between the mutually exclusive global and run final rebin made the UI interactions unnecessarily complicated and bug prone. It could also be confusing to users what the current state of the rebin configuration was, since the reduction table had a column for the rebin Q-step, but this was only used when run rebin was enabled and blank when global rebin was enabled.
This PR:
Binning Typedrop down menu, to better reflect that these are mutually exclusive in the Mantid algorithmMagnetismReflectometryReductionCheck all that apply:
References:
(instructions to set up the environment)
Start the GUI, load some data and verify that the reflectivity plot is updated correctly and consistently when the binning options are changed.
Check list for the reviewer
or an explanation is provided as to why release notes are unnecessary