-
Notifications
You must be signed in to change notification settings - Fork 6
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Rob/mdanse trajectory filter #615
base: protos
Are you sure you want to change the base?
Conversation
MDANSE/Src/MDANSE/Framework/Configurators/TrajectoryFilterConfigurator.py
Outdated
Show resolved
Hide resolved
a042cee
to
f3df3b0
Compare
b24005c
to
41b0d3e
Compare
MDANSE/Src/MDANSE/Framework/Configurators/TrajectoryFilterConfigurator.py
Outdated
Show resolved
Hide resolved
MDANSE/Src/MDANSE/Framework/Configurators/TrajectoryFilterConfigurator.py
Outdated
Show resolved
Hide resolved
MDANSE/Src/MDANSE/Framework/Configurators/TrajectoryFilterConfigurator.py
Outdated
Show resolved
Hide resolved
MDANSE/Src/MDANSE/Framework/Configurators/TrajectoryFilterConfigurator.py
Outdated
Show resolved
Hide resolved
MDANSE/Src/MDANSE/Framework/Configurators/TrajectoryFilterConfigurator.py
Outdated
Show resolved
Hide resolved
MDANSE_GUI/Src/MDANSE_GUI/InputWidgets/TrajectoryFilterWidget.py
Outdated
Show resolved
Hide resolved
if on: | ||
self.bound_freq_widget.setEnabled(True) | ||
return | ||
self.bound_freq_widget.setEnabled(False) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if on: | |
self.bound_freq_widget.setEnabled(True) | |
return | |
self.bound_freq_widget.setEnabled(False) | |
self.bound_freq_widget.setEnabled(on) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In fact this function could just be:
toggle_bound_frequencies = self.bound_freq_widget.setEnabled
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change loses expected behaviour, tested on a bandpass filter which requires the frequency other bound to become enabled and it does not.
MDANSE_GUI/Src/MDANSE_GUI/InputWidgets/TrajectoryFilterWidget.py
Outdated
Show resolved
Hide resolved
MDANSE_GUI/Src/MDANSE_GUI/InputWidgets/TrajectoryFilterWidget.py
Outdated
Show resolved
Hide resolved
MDANSE_GUI/Src/MDANSE_GUI/InputWidgets/TrajectoryFilterWidget.py
Outdated
Show resolved
Hide resolved
7afa869
to
97b6ec8
Compare
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. |
…ydrogen) atomic trajectory csv data for testing
…tion of filter object
8495046
to
68377e1
Compare
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:
|
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. |
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
UI &/or Backend
Backend
Tests
Other
Fixes
A list of fixes.
To test
Please describe the tests that were run to verify the changes.