-
Notifications
You must be signed in to change notification settings - Fork 55
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
[MRG] New widget to adjust default smoothing value in gui #924
[MRG] New widget to adjust default smoothing value in gui #924
Conversation
8fe7718
to
0fde5c0
Compare
hnn_core/gui/_viz_manager.py
Outdated
@@ -937,6 +944,11 @@ def data(self): | |||
"figs": self.figs | |||
} | |||
|
|||
@property | |||
def fig_default_params(self): |
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.
Does this need to be @property
? Seems like the other ones were created to regroup existing attributes for code cleanliness, whereas here you're just returning self.fig_defaults
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.
I made it a @Property so I could call it in the test:
viz_smooth_value = gui.viz_manager.fig_default_params['default_smoothing']
gui_smooth_value = gui.fig_default_params['default_smoothing'] | ||
viz_smooth_value = gui.viz_manager.fig_default_params['default_smoothing'] | ||
|
||
assert gui_smooth_value == new_smoothing |
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.
It'd be useful to add a similar assert for the original smooth values of the fig_default
and viz_manager
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.
I think I follow you, just updated 👍
Remove commented out code Co-authored-by: Nicholas Tolley <[email protected]>
…e everywhere per the suggestion from @ntolley
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.
LGTM. Tested and works, even for subsequent runs in-session
Title says it all