[MRG]: Better argument names for add_tonic_bias()

[kenloi]

Closes #347
For the add_tonic_bias() method in the network class in network.py, I changed the T arg to tstop. In the method, I replaced all the T instances with the new tstop arg.
The _add_drives_from_params(net) function in drives.py calls the add_tonic_bias() method; thus I changed the args to match my changes to add_tonic_bias().
I didn't find any other instances where the add_tonic_bias() method was used so it should be fixed.

[ntolley]

Great work @kenloi! I'll review the code soon, but you should check out the error log from travis. It seems the test_network,py is failing. I believe this is because you need to replace T with tstop in the tests as well.

[ntolley]

Maybe make self.cell_response.times[-1] a variable like t_end.

[ntolley]

The error is actually a bit deep in the code base. Since the dictionary key is being changed to tstop, it's actually breaking some code downstream:

hnn-core/hnn_core/network_builder.py

Line 391 in f84f3cf

cell.create_tonic_bias(**self.net.external_biases

Unpacks the dictionary such the key is a named argument. This means:

hnn-core/hnn_core/cell.py

Line 433 in f84f3cf

def create_tonic_bias(self, amplitude, t0, T, loc=0.5):

Is getting tstop as an argument, so you'll need to change the variable name there as well.

I'd recommend trying to run some simulations to make sure everything is functioning smoothly. You can copy code from the gamma example to test the function in a separate notebook.

[jasmainak]

@kenloi when you make a fix, you should run tests locally rather than wait for Travis CI to fail. You can do that using:

$ py.test .

If you want to be be dropped into the pdb debugger when an error happens you can do:

$ py.test . --pdb

Doing this has saved me a lot of time and it's why the unit tests exist!

[kenloi]

@ntolley Thanks for the pointers. I'll go through it again.

@jasmainak Thank you for the suggestion. I'll make sure to run tests locally before submitting.

[jasmainak]

perfect, let us know when you push the next update!

[codecov-commenter]

Codecov Report

Merging #354 (50b442f) into master (c65bc1e) will increase coverage by 0.02%.
The diff coverage is 40.00%.

❗ Current head 50b442f differs from pull request most recent head 9f3eb6e. Consider uploading reports for the commit 9f3eb6e to get more accurate results

@@            Coverage Diff             @@
##           master     #354      +/-   ##
==========================================
+ Coverage   90.94%   90.96%   +0.02%     
==========================================
  Files          13       13              
  Lines        2540     2547       +7     
==========================================
+ Hits         2310     2317       +7     
  Misses        230      230              

Impacted Files	Coverage Δ
hnn_core/cell.py	97.25% <0.00%> (ø)
hnn_core/drives.py	92.25% <ø> (ø)
hnn_core/params.py	88.97% <ø> (ø)
hnn_core/network.py	93.17% <50.00%> (+0.02%)	⬆️
hnn_core/viz.py	90.93% <0.00%> (+0.12%)	⬆️

---

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 c65bc1e...9f3eb6e. Read the comment docs.

[jasmainak]

@kenloi once the PR is good to go for you, can you update the title to have the [MRG] as a prefix? Also, you'll need to update whats_new.rst. Since the signature of a public method has been changed, you need to add it to the API section.

Also, make sure to build the docs locally to see how the whats_new page renders:

$ make html-noplot

to build without running examples,

and/or

$ make html

to build everything. The html outputs will be inside the _build/ folder

[cjayb]

Hey @kenloi your last commit (20da1e5) included a bunch of unwanted files. No worries, here's what I'd do. Make a note of the intended changes for the last commit, then simply cut off the last commit using:

git reset --hard HEAD~1

Then re-implement the intended changes and git add them.

I recommend setting up an alias in your ~/.bashrc or equivalent

alias gad='git add -u'

Then gad . stages all changes, but doesn't add any previously untracked files. And make it a habit to check git status before running git commit.

[kenloi]

@cjayb Thanks for the suggestions. My earlier commit (48024b9), I ran both git status and git diff to check which files to add and commit. I only edited the whats_new.rst file to update the docs, but then the tests started failing, reporting that it couldn't find the module 'mne'. This was a surprise given that I had passed all the tests in the commit before (ca91da4).

I tried looking up the module from my WSL bash shell and got the same ModuleNotFoundError. I opened a python IDE from powershell and tried importing mne there but I also got the same ModuleNotFoundError. Only after running a pip install mne and downloading the module into my WSL bash shell environment did my tests start passing locally. I thought maybe I was missing some files so I sent a git add --all and pushed another commit and now the tests are failing again.

I'll give your suggestions a try to see if that fixes the problem. Any ideas as to what may have happened between my functioning commit and my updates to the documentation?

[cjayb]

Only after running a pip install mne and downloading the module into my WSL bash shell environment did my tests start passing locally

This makes sense. It's not super well-documented, but the somato-example requires mne. So pip-installing it into your dev environment was the right thing to do.

I thought maybe I was missing some files so I sent a git add --all

This was the fatal mistake: the --all added the entire doc/-folder into the subsequent commit (not just the intended whats_new.rst. A lot of cruft got included from your make html. You should just do the hard reset I mentioned above (make a copy of whats_new on your desktop or smth, so it's easy to copy the changes back after the reset).

Once you've reset & committed only the desired changes, you'll have to do

git push -f

to force GitHub to accept the branch: the reset is going to make a non-linear change the git history. But in this case it's what you want.

[kenloi]

@jasmainak I've submitted the final commit, updated the api docs, and fixed the title prefix to [MRG]. Please let me know if there's anything else and thank you for the guidance.

@cjayb Thank you for your input!

[jasmainak]

Suggested change

- `Kenneth Loi`_

this is contributors for 0.1 release. We'll add you to the contributor list for 0.2 :)

[jasmainak]

Looks good other than minor nitpick

[kenloi]

@jasmainak no problem! I can wait :)

[jasmainak]

Congratulations @kenloi on your first contribution! 🎉

what's the next PR? :)

[jasmainak]

maybe #306 is an easy one to knock off and get more comfortable with git while we find you one that is more "intermediate" level

[kenloi]

Thanks @jasmainak! #306 sounds good, I’ll give it a try.