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

Changes read_spikes #207

Merged
merged 6 commits into from Jan 15, 2021
Merged

Conversation

samikane
Copy link
Contributor

@samikane samikane commented Nov 16, 2020

This should change the read_spikes function to address issue_178. I'm not sure how to drop the change I made to the testing file, so please get rid of that somehow.

closes #178

@jasmainak
Copy link
Collaborator

@samikane next step is to add a unit test :)

@rythorpe
Copy link
Contributor

@samikane my recommendation would be to add a round-trip test to test_network.test_spikes() where you instantiate an empty Spikes object, write it to a temporary file, read it back in, and assert that an empty Spikes object is successfully created. See here for inspiration.

Comment on lines 51 to 52
raise ValueError("gid_dict must be provided if spike types "
"are unspecified in the file %s" % (file,))
Copy link
Contributor

Choose a reason for hiding this comment

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

This is probably causing your flake8 error.

Copy link
Contributor

Choose a reason for hiding this comment

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

Just checked the travis build, there's a couple more issues:
image

@samikane these should be pretty quick to fix, you can enable flake8 in the settings of your IDE (VS code?) to catch these before pushing. Also if checks are failing you can click on them and look at the traceback to see the errors.

Satisfying flake8 while making the code readable can be more art than science at times...

@jasmainak
Copy link
Collaborator

I edited the PR description so it references the original issue.

What's the roadblock here?

@codecov-io
Copy link

codecov-io commented Nov 25, 2020

Codecov Report

Merging #207 (871e4bf) into master (93c79d9) will decrease coverage by 0.05%.
The diff coverage is 28.57%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #207      +/-   ##
==========================================
- Coverage   66.85%   66.79%   -0.06%     
==========================================
  Files          21       21              
  Lines        2290     2298       +8     
==========================================
+ Hits         1531     1535       +4     
- Misses        759      763       +4     
Impacted Files Coverage Δ
hnn_core/network.py 49.63% <0.00%> (-0.73%) ⬇️
hnn_core/tests/test_network.py 100.00% <100.00%> (ø)

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 93c79d9...871e4bf. Read the comment docs.

@jasmainak jasmainak added this to the 0.1 milestone Nov 30, 2020
@jasmainak
Copy link
Collaborator

@samikane do you still plan to get to this?

I can take over or @rythorpe can if you don't have time

@samikane
Copy link
Contributor Author

samikane commented Dec 24, 2020 via email

@rythorpe
Copy link
Contributor

rythorpe commented Jan 4, 2021

@jasmainak stay tuned for a few remaining commits following @samikane and my meeting tomorrow.

@samikane
Copy link
Contributor Author

samikane commented Jan 6, 2021

@jasmainak update to the pull request is ready for your review. We've added a test

@jasmainak
Copy link
Collaborator

Diff looks good

Can you update whats_new.rst and fix the flake8. Then I'll merge

@samikane
Copy link
Contributor Author

@jasmainak should be ready to merge now

@jasmainak jasmainak merged commit 69006c4 into jonescompneurolab:master Jan 15, 2021
@jasmainak
Copy link
Collaborator

Looks great, merged. You did it @samikane ! Congratulations 🎉 🍻

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.

read_spikes doesn't handle an empty file properly
5 participants