-
Notifications
You must be signed in to change notification settings - Fork 52
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] Added plot sets functionality #746
Conversation
@gtdang give it a quick review and let me know your thoughts |
Codecov ReportAttention: Patch coverage is
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## master #746 +/- ##
==========================================
- Coverage 92.67% 92.62% -0.06%
==========================================
Files 27 27
Lines 4928 4961 +33
==========================================
+ Hits 4567 4595 +28
- Misses 361 366 +5 ☔ View full report in Codecov by Sentry. |
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.
Looking good Camilo. Left some suggestions and questions.
@gtdang I let you iterate with @kmilo9999 and merge when happy! Don't forget to update |
hnn_core/gui/_viz_manager.py
Outdated
@@ -87,6 +144,11 @@ def check_sim_plot_types( | |||
target_selection.value = 'None' | |||
|
|||
|
|||
def check_template_type_is_data_dependant(template_name): |
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.
public function with no docstring
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.
Changed to private
@@ -87,6 +144,11 @@ def check_sim_plot_types( | |||
target_selection.value = 'None' | |||
|
|||
|
|||
def _check_template_type_is_data_dependant(template_name): |
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 should have a unit test
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.
@kmilo9999 do you think this is sufficiently covered by the current unit tests? It's a pretty trivial function so I'll leave it to your judgement
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.
@kmilo9999 do you think this is sufficiently covered by the current unit tests? It's a pretty trivial function so I'll leave it to your judgement
Yes, the unit test already covers this function inside the gui.viz_manager.make_fig_button
call.
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.
Thanks Camilo. This is looking really great! Make sure to update the whats_new.rst file mentioning this new feature.
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.
Suggested slight change to the what's new text to be more specific.
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.
Looks good to me!
Awesome. Let me clean the commit history before merging with main |
09971a4
to
e591583
Compare
@gtdang This one is ready to go! 👍 |
7f70b38
to
4d0d4a6
Compare
@gtdang I added the Dipole-Spike plot. Review it if you want. It is ready to go. |
@ntolley |
@kmilo9999 it seems like there are rebase conflicts? Happy to merge once those are fixed (i.e. running We can discuss in our Thursday meeting if there's too many conflicts |
STY: fixed typos in comments
…re in the dropdown DOC: Fixing gui layout template name STY: Fixing typo in a function naming
MAINT: changed new functions to private. Adding grid specs for new data templates
STY: Fixed typo in test docustring MAINT: pythonizing for loop on list of tuples DOC: Added plot sets feature to whats_new.rst docs: Removed ipywidgets version and added note pointing to GUI installation section.
61062f9
to
30a15ec
Compare
It was a small conflict in the |
Something weird happened in your rebase @kmilo9999 . It appears that 501d82b got added to your commit history, which is from a previous PR. |
@kmilo9999 please fetch master branch again and rebase ... it will get rid of the weird commits |
doc/whats_new.rst
Outdated
- Added kwargs options to `plot_spikes_hist` for adjusting the histogram plots | ||
of spiking activity, by `Abdul Samad Siddiqui`_ in :gh:`732`. |
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 shouldn't be in your PR ... it's already down below
30a15ec
to
327a3a6
Compare
@ntolley It should be ready to merge. |
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.
Great work @kmilo9999!! This is going to be a superl feature for the GUI
solves GUI plot sets #711