jonescompneurolab / hnn-core

Simulation and optimization of neural circuits for MEG/EEG source estimates
https://jonescompneurolab.github.io/hnn-core/
BSD 3-Clause "New" or "Revised" License
55 stars 52 forks source link

[MRG] ENH: Added instance type check for plot_dipole function and also a test for it #606

Closed raj1701 closed 1 year ago

raj1701 commented 1 year ago

Closes #511 I made changes in plot_dipole function in viz.py to type of argument 1. Iteration is used when a list of dipoles is passed. I have included a test in test_dipole.py where [dipole, integer] is passed as an argument to plot_dipole() and the appropriate error message is checked using pytest.

raj1701 commented 1 year ago

I have removed the flake8 errors

jasmainak commented 1 year ago

You should use the phrase "closes #511", otherwise the linked PR will not get closed when this PR is merged. See:

https://docs.github.com/en/issues/tracking-your-work-with-issues/linking-a-pull-request-to-an-issue#linking-a-pull-request-to-an-issue-using-a-keyword

codecov-commenter commented 1 year ago

Codecov Report

Merging #606 (f62a159) into master (d5d1a03) will decrease coverage by 0.07%. The diff coverage is 100.00%.

:exclamation: Current head f62a159 differs from pull request most recent head fc1e84c. Consider uploading reports for the commit fc1e84c to get more accurate results

:mega: This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@            Coverage Diff             @@
##           master     #606      +/-   ##
==========================================
- Coverage   92.19%   92.12%   -0.07%     
==========================================
  Files          22       22              
  Lines        4229     4231       +2     
==========================================
- Hits         3899     3898       -1     
- Misses        330      333       +3     
Impacted Files Coverage Δ
hnn_core/viz.py 89.23% <100.00%> (+0.04%) :arrow_up:

... and 1 file with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

jasmainak commented 1 year ago

Can you update whats_new.rst ?

raj1701 commented 1 year ago

Should I update in Current Changelog in whats_new.rst?

rythorpe commented 1 year ago

Also, you'll need to rebase @raj1701. Let me know if you need help with this.

raj1701 commented 1 year ago

I can give it a shot. I'll contact you if I get stuck.

raj1701 commented 1 year ago

@rythorpe I thinks I was successful with the rebase. Please check once. Thanks

rythorpe commented 1 year ago

You're almost done @raj1701, just a minor adjustment to the warning message!

jasmainak commented 1 year ago

Okay I simplified the logic a bit @raj1701 @rythorpe ... please take a look. It is slightly different from the original logic but I think it's not less correct. Wdyt?

raj1701 commented 1 year ago

I think its correct and much more simplified. Now I get what you were trying to convey @jasmainak.

jasmainak commented 1 year ago

@raj1701 try not to make too many commits for a simple change. It will make your life difficult with respect to rebasing. Make use of git commit --amend or squash the commits so each commit represents a logical unit of code.

rythorpe commented 1 year ago

Are we ready to rebase and merge @raj1701 @jasmainak?

raj1701 commented 1 year ago

@raj1701 try not to make too many commits for a simple change. It will make your life difficult with respect to rebasing. Make use of git commit --amend or squash the commits so each commit represents a logical unit of code.

Yes will remember this. I faced many rebase errors here also.

jasmainak commented 1 year ago

Yep, I changed the PR to MRG. Please squash and merge @rythorpe !

rythorpe commented 1 year ago

Thanks @raj1701 and @jasmainak!