Skip to content
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] GUI refactoring #500

Merged
merged 41 commits into from
Jul 24, 2022
Merged

[MRG] GUI refactoring #500

merged 41 commits into from
Jul 24, 2022

Conversation

chenghuzi
Copy link
Collaborator

Try to refactor the GUI code in an OP manner.

@codecov-commenter
Copy link

codecov-commenter commented Jul 13, 2022

Codecov Report

Merging #500 (ccd1ce6) into master (b630f93) will increase coverage by 2.18%.
The diff coverage is 89.68%.

❗ Current head ccd1ce6 differs from pull request most recent head 63f92e5. Consider uploading reports for the commit 63f92e5 to get more accurate results

@@            Coverage Diff             @@
##           master     #500      +/-   ##
==========================================
+ Coverage   87.74%   89.93%   +2.18%     
==========================================
  Files          19       20       +1     
  Lines        3828     3883      +55     
==========================================
+ Hits         3359     3492     +133     
+ Misses        469      391      -78     
Impacted Files Coverage Δ
hnn_core/gui/gui.py 84.52% <89.63%> (+21.99%) ⬆️
hnn_core/gui/__init__.py 100.00% <100.00%> (ø)
hnn_core/parallel_backends.py 82.32% <0.00%> (+0.82%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b630f93...63f92e5. Read the comment docs.

hnn_core/gui/gui.py Outdated Show resolved Hide resolved
hnn_core/gui/gui.py Outdated Show resolved Hide resolved
@jasmainak
Copy link
Collaborator

@chenghuzi I was looking at tqdm and it seems they also have a notebook for the tests of the notebook: https://github.com/tqdm/tqdm/blob/4591083884c8ca079ccd1192c079113c7baeadc8/tests_notebook.ipynb. I think that might be the simplest solution for us to avoid complicated frameworks ...

@jasmainak
Copy link
Collaborator

see how the Makefile is used to make the test commands consistent across different CIs and developers: https://github.com/tqdm/tqdm/blob/4f208e72552c4d916aa4fe6a955349ee8b2ed353/Makefile#L65

@chenghuzi
Copy link
Collaborator Author

@chenghuzi I was looking at tqdm and it seems they also have a notebook for the tests of the notebook: https://github.com/tqdm/tqdm/blob/4591083884c8ca079ccd1192c079113c7baeadc8/tests_notebook.ipynb. I think that might be the simplest solution for us to avoid complicated frameworks ...

I think probably we can directly call this in a . py file. I'll submit a commit about this soon

hnn_core/gui/gui.py Outdated Show resolved Hide resolved
hnn_core/gui/gui.py Outdated Show resolved Hide resolved
hnn_core/gui/gui.py Outdated Show resolved Hide resolved
@jasmainak
Copy link
Collaborator

How do you plan to use the class and the methods in the documentation? Let's say you are describing the part where drives must be added? You would want to:

a) activate the drive tab
b) click "add drive"
c) select drive parameters
d) show the current state of GUI (i.e., re-paint the layout/GUI)

is that something that can be done with the current interface?

@jasmainak
Copy link
Collaborator

@chenghuzi you need to update api.rst

hnn_core/gui/gui.py Outdated Show resolved Hide resolved
hnn_core/gui/gui.py Outdated Show resolved Hide resolved
@chenghuzi chenghuzi mentioned this pull request Jul 19, 2022
8 tasks
@chenghuzi
Copy link
Collaborator Author

We have dynamic connectivity tab now in 8c920a2

@chenghuzi
Copy link
Collaborator Author

@chenghuzi you need to update api.rst

Updated in c39810a

@jasmainak
Copy link
Collaborator

should the code logic for finding possible connectivity pairs between cell types be in a separate function? I can imagine this logic could be useful outside of the GUI as well.

@chenghuzi
Copy link
Collaborator Author

should the code logic for finding possible connectivity pairs between cell types be in a separate function? I can imagine this logic could be useful outside of the GUI as well.

Agreed. But we may need another PR for this because the current way we use to extract connections is hierarchical while a general method (possibly in the form of a method in Network) should be plain. And then we can further simplify the existing code for extracting possible connectivity boxes using that method.

hnn_core/gui/gui.py Outdated Show resolved Hide resolved
@chenghuzi
Copy link
Collaborator Author

variables is now simulation_data. We keep that because it allows modifying the item in the dict easily, otherwise we need to return the value to modify them

@jasmainak
Copy link
Collaborator

Great. I'm doing a pass over the code right now. Report back in an hour :-)

@jasmainak jasmainak force-pushed the hnn_gui branch 2 times, most recently from 5e0cb9f to d37bc45 Compare July 21, 2022 17:17
@jasmainak
Copy link
Collaborator

okay back to you. Finish this PR: https://github.com/chenghuzi/hnn-core/pull/2/files

and I think we're good to go here.

I also had some trouble following the logic of loading the param files. But let's discuss that later.

hnn_core/gui/gui.py Outdated Show resolved Hide resolved
@chenghuzi
Copy link
Collaborator Author

chenghuzi commented Jul 24, 2022

okay back to you. Finish this PR: https://github.com/chenghuzi/hnn-core/pull/2/files

and I think we're good to go here.

I also had some trouble following the logic of loading the param files. But let's discuss that later.

All format issues fixed, could I merge this now @jasmainak ?

@@ -943,7 +943,8 @@ def on_upload_change(change, params, tstop, dt, log_out, simulation_data,
with log_out:
print(
"Same param. No reloading."
"To force reloading, hit \"clear uploaded parameters\" button")
"To force reloading, hit \"clear uploaded parameters\" button",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see: https://peps.python.org/pep-0008/#string-quotes

"When a string contains single or double quote characters, however, use the other one to avoid backslashes in the string. It improves readability."

Just avoid backslashes when you can :)

@jasmainak jasmainak merged commit f5d1691 into jonescompneurolab:master Jul 24, 2022
@jasmainak
Copy link
Collaborator

Never merge your own PR :) But I think it's time to move on to the next steps! Thanks @chenghuzi . This is a great improvement 🥳 🥳 🥳 . Next step is to try and figure out automatic building of documentation using html2canvas and jupyter-require ?

@chenghuzi
Copy link
Collaborator Author

okay back to you. Finish this PR: https://github.com/chenghuzi/hnn-core/pull/2/files
and I think we're good to go here.
I also had some trouble following the logic of loading the param files. But let's discuss that later.

All format issues fixed

Never merge your own PR :) But I think it's time to move on to the next steps! Thanks @chenghuzi . This is a great improvement 🥳 🥳 🥳 . Next step is to try and figure out automatic building of documentation using html2canvas and jupyter-require ?

I figured out a way to do that, though not every elegant. Will open a PR for that soon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants