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.

Sign up for GitHub

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

Jump to bottom

[MRG] make cell_response.write more robust #456

Merged
merged 4 commits into from Jan 24, 2022
Merged

[MRG] make cell_response.write more robust #456

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
25cc5a3
MAINT: Allow paths without formatting.
jasmainak Jan 13, 2022
32755f1
Flake8
jasmainak Jan 13, 2022
2f6813e
DOC: update whats_new
jasmainak Jan 13, 2022
cd14470
ENH: more verbose to not surprise user
jasmainak Jan 19, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 3 additions & 0 deletions doc/whats_new.rst
View file
Open in desktop
Expand Up @@ -16,6 +16,9 @@ Changelog
Bug
~~~

- Allow regular strings as filenames in :meth:`~hnn_core.Cell_response.write` by
`Mainak Jas`_ in :gh:456.

API
~~~

Expand Down
22 changes: 19 additions & 3 deletions hnn_core/cell_response.py
View file
Open in desktop
Expand Up @@ -376,8 +376,10 @@ def write(self, fname):
Parameters
----------
fname : str
String format (e.g., '<pathname>/spk_%d.txt') of the
path to the output spike file(s).
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.
Comment on lines +379 to +382
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.


Outputs
-------
Expand All @@ -387,9 +389,23 @@ def write(self, fname):
2) spike gid, and
3) gid type
"""
fname = str(fname)
old_style = True
try:
fname % 0
except TypeError:
fname.format(0)
old_style = False
except TypeError:
fname.replace('.txt', '_%d.txt')

for trial_idx in range(len(self._spike_times)):
with open(str(fname) % (trial_idx,), 'w') as f:
if old_style:
this_fname = fname % (trial_idx,)
else:
this_fname = fname.format(trial_idx)
print(f'Writing file {this_fname}')
with open(this_fname, 'w') as f:
for spike_idx in range(len(self._spike_times[trial_idx])):
f.write('{:.3f}\t{}\t{}\n'.format(
self._spike_times[trial_idx][spike_idx],
Expand Down
2 changes: 2 additions & 0 deletions hnn_core/tests/test_cell_response.py
View file
Open in desktop
Expand Up @@ -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'))
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this overwrite the files? Then L49 is only testing the previous line

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yeah the others are smoke tests

assert empty_spike == read_spikes(tmpdir.join('empty_spk_*.txt'))

assert ("CellResponse | 2 simulation trials" in repr(empty_spike))
Expand Down