ISISNeutronMuon
Rob/mdanse trajectory filter
Description of work TODO:
(Probably don't need to solve all of these immediately - some may become issues for further work after this is completed)
UI
- [x] Refactor UI to fix bug on reloading Filter Designer after changing trajectory
- [x] Odd crash occurs when returning to TrajectoryFilter job page from another job, error appears to be due to deleted grid layout components in the trajectory filter widget
- [ ] Trajectory filter widget panel inputs move around alot in layout - will change to a grid layout for all input components to improve stability
- [x] Allow user to visualise the attenuated trajectory power spectrum alongside filter (currently non-enabled "Show attenuation" checkbox)
- [x] When x-axis is set to energy units, the axis contracts - this should remain the same scale as for frequency units
UI &/or Backend
- [x] Number of simulation frames post-filtering exceeds original frame count
- [x] Investigate cause of atoms collapsing to 'ugly blob' when certain filter variants (seems to be highpass and bandpass removing low frequency vibrations) are applied to trajectory
- [x] Ensure the Z- and S- domain representations of the transfer function polynomial are correct
Backend
- [x] Filter class and filter-variant child classes
- [x] Write metadata (filter configuration)
Tests
- [x] Basic unit tests for correct functioning of filter class
- [ ] Full end-to-end test for a more complex molecular dynamics trajectory
Other
- [ ] Ensure sensible default values for filters
- [x] Complete numpy docstrings for methods
- [x] Apply black formatting
Fixes A list of fixes.
To test Please describe the tests that were run to verify the changes.
It would be good to have a way of adjusting the view in the filter designer window. I have some use cases where the relevant part of the data is all in the corner.
Probably just a standard matplotlib toolbar could be added to control behaviour like zooming in and out. PlotWidget.py does this using NavigationToolbar2QT.
Also, let me see if I got this right: the instrument resolution and projection inputs affect the power spectrum in the preview - but do they affect the results produced by the filter?
Also, let me see if I got this right: the instrument resolution and projection inputs affect the power spectrum in the preview - but do they affect the results produced by the filter?
No effect on the filtered trajectory, only required for the power spectrum calculation.
I think that problems can still occur if you run the filter for a trajectory, and then load a different trajectory and switch to it. I lost the ability to switch from "lowpass" to other modes. I changed the filter type and then the other options were still in the combo box, but they would trigger this error output:
2025-03-06 12:28:43,221 - ERROR - process[22318] - main 35 - EXCEPTION:
<class 'RuntimeError'>
wrapped C/C++ object of type QAction has been deleted
<traceback object at 0x300c830c0>
Traceback (most recent call last):
File "MDANSE_GUI/InputWidgets/TrajectoryFilterWidget.py", line 412, in <lambda>
signal.connect(lambda val: self.edit_current_filter(setting_key, val))
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "MDANSE_GUI/InputWidgets/TrajectoryFilterWidget.py", line 239, in edit_current_filter
self.render_canvas_assets()
File "MDANSE_GUI/InputWidgets/TrajectoryFilterWidget.py", line 762, in render_canvas_assets
self.render_graph(
File "MDANSE_GUI/InputWidgets/TrajectoryFilterWidget.py", line 660, in render_graph
self._figure.clear()
File "lib/python3.11/site-packages/matplotlib/figure.py", line 3133, in clear
toolbar.update()
File "lib/python3.11/site-packages/matplotlib/backend_bases.py", line 3281, in update
self.set_history_buttons()
File "lib/python3.11/site-packages/matplotlib/backends/backend_qt.py", line 847, in set_history_buttons
self._actions['back'].setEnabled(can_backward)
RuntimeError: wrapped C/C++ object of type QAction has been deleted
I think that problems can still occur if you run the filter for a trajectory, and then load a different trajectory and switch to it. I lost the ability to switch from "lowpass" to other modes. I changed the filter type and then the other options were still in the combo box, but they would trigger this error output:
2025-03-06 12:28:43,221 - ERROR - process[22318] - main 35 - EXCEPTION: <class 'RuntimeError'> wrapped C/C++ object of type QAction has been deleted <traceback object at 0x300c830c0> Traceback (most recent call last): File "MDANSE_GUI/InputWidgets/TrajectoryFilterWidget.py", line 412, in <lambda> signal.connect(lambda val: self.edit_current_filter(setting_key, val)) ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ File "MDANSE_GUI/InputWidgets/TrajectoryFilterWidget.py", line 239, in edit_current_filter self.render_canvas_assets() File "MDANSE_GUI/InputWidgets/TrajectoryFilterWidget.py", line 762, in render_canvas_assets self.render_graph( File "MDANSE_GUI/InputWidgets/TrajectoryFilterWidget.py", line 660, in render_graph self._figure.clear() File "lib/python3.11/site-packages/matplotlib/figure.py", line 3133, in clear toolbar.update() File "lib/python3.11/site-packages/matplotlib/backend_bases.py", line 3281, in update self.set_history_buttons() File "lib/python3.11/site-packages/matplotlib/backends/backend_qt.py", line 847, in set_history_buttons self._actions['back'].setEnabled(can_backward) RuntimeError: wrapped C/C++ object of type QAction has been deleted
Looks to be related to the recently introduced toolbar for the filter designer, I'll look into it
One more thing: the spin boxes in the filter dialog could be made less restrictive. One of them does not allow values lower than 1.0, and the step size is also quite large. This is still OK most of the time, but we cannot exclude the possibility that for some trajectory this will stop the user from applying the filter they wanted.
Would it make sense to check the energy axis of the position power spectrum, and then set the step of the spin box to a single step on the energy axis? Also, if it is important not to allow 0 as an input in that spin box, the minimum could be set to the first non-zero energy value.
It is still possible to find settings which will make the trajectory collapse to a point, or at least make atoms overlap.
Here is a test on the CO2 trajectory:
Maybe the check whether to apply the offsets could be based on actual atom positions after filtering? E.g. compare the first frame before filtering to the first frame after, check the span of the coordinates, and if the total size of the system after filtering is e.g. less than 80% of the original, then offsets need to be applied to bring the atoms back to their positions.
Another question: the spin boxes are now restricted to specific values, and snap to the nearest value as expected. However:
- 0 is never allowed. Would it make sense to allow it for any filter type?
- The step of the spin box seems to be 1.5 of the step in the vibrational DOS. Was it supposed to be equal to the energy axis step?
The DOS plot with points every 42 meV:
The spin box with values every 63 meV:
It is still possible to find settings which will make the trajectory collapse to a point, or at least make atoms overlap.
Here is a test on the CO2 trajectory:
Maybe the check whether to apply the offsets could be based on actual atom positions after filtering? E.g. compare the first frame before filtering to the first frame after, check the span of the coordinates, and if the total size of the system after filtering is e.g. less than 80% of the original, then offsets need to be applied to bring the atoms back to their positions.
Yes, good catch. It seems that in this case, setting the ChebyshevTypeII filter order to 2 means the frequency response is neither 1 nor zero at w = 0 (closer to 0.1). I hadn't realised this yet, but this means that in this case the initial positions are not being applied correctly, resulting in the empty box. I think it may well be a good idea to - as you suggest - apply necessary offsets based on the difference between atomic positions at t0, before and after filtering.
@MBartkowiakSTFC
0 is never allowed. Would it make sense to allow it for any filter type?
The scipy filters generally do not accept zero-valued critical frequencies. There may be odd cases where zero is valid, but this would depend on the filter type, and would invovle some dynamic spinbox limits.
The step of the spin box seems to be 1.5 of the step in the vibrational DOS. Was it supposed to be equal to the energy axis step? The DOS plot with points every 42 meV:
The units of the cutoff freq spinbox step is always in the frequency units used to display the graph x-axis (so, either rad/ps or THz, depending on the filter) - never meV, although the energy-converted information is displayed in the text panel below the graph.
The scipy filters generally do not accept zero-valued critical frequencies. There may be odd cases where zero is valid, but this would depend on the filter type, and would invovle some dynamic spinbox limits.
OK, good to know.
The units of the
cutoff freqspinbox step is always in the frequency units used to display the graph x-axis (so, either rad/ps or THz, depending on the filter) - never meV, although the energy-converted information is displayed in the text panel below the graph.
I would not mind making this slightly clearer in the future, but this can easily be dealt with in some later PR.
I think that the unit tests take too long in their current implementation. For me it would be OK to spend up to 20 seconds running all the tests of TrajectoryFilter.
I would vote for removing the DUT49 trajectory altogether, and in the other ones you could remove most of the atoms from the trajectory. This way the time axis will still be the same and there will be no need to change the input parameters. The reference output files would still have to be adjusted.
OK, the TrajectoryFilter tests now take about 11 seconds when I run them.