[MRG] make cell_response.write more robust #456
Conversation
jasmainak
changed the title
Codecov Report
@@ Coverage Diff @@
## master #456 +/- ##
==========================================
- Coverage 88.51% 88.49% -0.02%
==========================================
Files 18 18
Lines 3316 3329 +13
==========================================
+ Hits 2935 2946 +11
- Misses 381 383 +2
Continue to review full report at Codecov.
|
| String format (e.g., 'spk_%d.txt' or 'spk_{0}.txt') of the | ||
| path to the output spike file(s). If no string format | ||
| is provided, the trial index will be automatically | ||
| appended to the file name. |
There was a problem hiding this comment.
So it is now impossible for a user to specify a filename without a trial number contained somewhere in it? I get that this prevents the user from accidentally overwriting multiples trials, however, I'm not sure how I feel about automatically modifying user input. Feel free to override this opinion as I see a valid argument either way.
There was a problem hiding this comment.
It was not possible before this PR also. This PR just automatically appends a trial number if the user doesn't specify it. Other option is to throw an explicit error (as opposed to the cryptic error we get now) if the user provides a filename without a format specifier when there are multiple trials.
There was a problem hiding this comment.
I'm leaning towards the explicit error being best. Especially since you're writing the full file name 'blah.txt' as opposed to a generic name with no '.txt'at the end.
I think it'd be ok if the user use provided 'filenamehere' and then the '_1.txt' was appended automatically.
| @@ -44,6 +44,8 @@ def test_cell_response(tmpdir): | |||
| empty_spike = CellResponse(spike_times=[[], []], spike_gids=[[], []], | |||
| spike_types=[[], []]) | |||
| empty_spike.write(tmpdir.join('empty_spk_%d.txt')) | |||
| empty_spike.write(tmpdir.join('empty_spk.txt')) | |||
| empty_spike.write(tmpdir.join('empty_spk_{0}.txt')) | |||
There was a problem hiding this comment.
Does this overwrite the files? Then L49 is only testing the previous line
There was a problem hiding this comment.
yeah the others are smoke tests
This is a good solution, thanks @jasmainak! |
I was just trying to use this with Michael Hines and realized that it throws an error for a regular string. I think it should instead append the trial index instead of throwing an error.