-
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
…ydrogen) atomic trajectory csv data for testing
…tion of filter object
…attenuation + fix freq -> energy conversion
…lowpass (which results in removing them)
a042cee
to
f3df3b0
Compare
b24005c
to
41b0d3e
Compare
settings_dict = dict() | ||
for setting, values in filter.default_settings.items(): | ||
settings_dict.update({setting: values["value"]}) | ||
return settings_dict |
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.
settings_dict = dict() | |
for setting, values in filter.default_settings.items(): | |
settings_dict.update({setting: values["value"]}) | |
return settings_dict | |
return {setting: values["value"] for setting, values in filter.default_settings.items()} |
Will do and be faster.
return ( | ||
'{ "filter": "' | ||
+ f'{filter.__name__}"' | ||
+ ", " | ||
+ '"attributes": ' | ||
+ f"{json.dumps(settings)}" | ||
+ "}" | ||
) |
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.
Any particular reason to use json.dumps
(i.e. do you need False
-> false
, None
-> null
)?
return ( | |
'{ "filter": "' | |
+ f'{filter.__name__}"' | |
+ ", " | |
+ '"attributes": ' | |
+ f"{json.dumps(settings)}" | |
+ "}" | |
) | |
return repr({"filter": filter.__name__, "attributes": settings}) |
If you need json.dumps
return ( | |
'{ "filter": "' | |
+ f'{filter.__name__}"' | |
+ ", " | |
+ '"attributes": ' | |
+ f"{json.dumps(settings)}" | |
+ "}" | |
) | |
return json.dumps({"filter": filter.__name__, "attributes": settings}) |
Alternatively, if you want this f-string style, why not
return ( | |
'{ "filter": "' | |
+ f'{filter.__name__}"' | |
+ ", " | |
+ '"attributes": ' | |
+ f"{json.dumps(settings)}" | |
+ "}" | |
) | |
return f'{{ "filter": "{filter.__name__}", "attributes": {json.dumps(settings)}}}' |
settings_dict.update({setting: values["value"]}) | ||
return settings_dict | ||
|
||
_settings = filter_default_attributes.__func__(object()) |
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.
Mixing class-level properties and methods will probably cause confusion in future.
@property | ||
def settings(self): | ||
return self._settings | ||
|
||
@settings.setter | ||
def settings(self, settings: dict): | ||
self._settings = settings |
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.
Why do these need to be @property
s if they behave like standard attributes? Are they over-ridden on inheritance?
dict_value = json.loads(value) | ||
|
||
try: | ||
{"filter", "attributes"} in set(dict_value.keys()) |
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.
Dictionary keys already behave like sets
, no need to cast. Also, are you looking for:
{"filter", "attributes"} in set(dict_value.keys()) | |
{"filter", "attributes"} < dict_value.keys() |
response = filter.freq_response | ||
|
||
# Lambda to resample and normalise input values | ||
values = lambda a, new_len: signal.resample(a, new_len) * (a.max() ** (-1)) |
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.
Considered bad practice to assign a lambda
.
Always use a def statement instead of an assignment statement that binds a lambda expression directly to an identifier
-- PEP-8
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
self._figure_info.append(f" ") | ||
self._figure_info.append(f" ") | ||
|
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.
self._figure_info.append(f" ") | |
self._figure_info.append(f" ") | |
self._figure_info.append(" ") | |
self._figure_info.append(" ") | |
{ | ||
key: filter.default_settings[key]["value"] | ||
for key in missing | ||
if key in defaults.keys() |
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 key in defaults.keys() | |
if key in defaults |
@staticmethod | ||
def combine_attributes(filter: Filter, attributes: dict) -> dict: |
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.
Not sure why this isn't a method on 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
UI &/or Backend
Backend
Tests
Other
Fixes
A list of fixes.
To test
Please describe the tests that were run to verify the changes.