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

Use pandas if available #422

Merged
merged 7 commits into from Aug 3, 2016

Conversation

Projects
None yet
3 participants
@sanjayankur31
Contributor

sanjayankur31 commented Jul 15, 2016

It tries to load the pandas module and uses it if it can, otherwise, it
falls back to numpy.

Note that the nest.is_iterable test returns True for a string, since a
string is iterable and the function then tries to iterate over each
character in the string! I've, therefore, changed the test.

Tested this out with the gdf file generated from
examples/nest/tsodyks_shortterm_bursts.sli using the
examples/nest/plot_tsodyks_shortterm_bursts.py plotter script and it
works both with and without pandas.

Fixes #400 to begin with.

Use pandas if available
It tries to load the pandas module and uses it if it can, otherwise, it
falls back to numpy.

Note that the nest.is_iterable test returns True for a string, since a
string is iterable and the function then tries to iterate over each
character in the string! I've, therefore, changed the test.

Tested this out with the gdf file generated from
`examples/nest/tsodyks_shortterm_bursts.sli` using the
`examples/nest/plot_tsodyks_shortterm_bursts.py` plotter script and it
works both with and without pandas.

Fixes #400 to begin with.
@apeyser

This comment has been minimized.

Show comment
Hide comment
@apeyser

apeyser Jul 25, 2016

Contributor

@sanjayankur31

Maybe the test for ["string"...] should be inverted? We should check for isinstance(str) for the single filename case rather than checking that we received a list or a tuple.

In fact, the code can be simplified by just making

if isinstance(fname, str):
    fname = (fname,)

but maybe there's a python2.7 issue with str vs byte vs unicode types for the test?

Contributor

apeyser commented Jul 25, 2016

@sanjayankur31

Maybe the test for ["string"...] should be inverted? We should check for isinstance(str) for the single filename case rather than checking that we received a list or a tuple.

In fact, the code can be simplified by just making

if isinstance(fname, str):
    fname = (fname,)

but maybe there's a python2.7 issue with str vs byte vs unicode types for the test?

@sanjayankur31

This comment has been minimized.

Show comment
Hide comment
@sanjayankur31

sanjayankur31 Jul 25, 2016

Contributor

Working on this now!

Contributor

sanjayankur31 commented Jul 25, 2016

Working on this now!

Improve filename checks
Checks to see if filenames are strings, or list or tuples of strings.

In py2, strings by default are bytestrings, not unicode strings - in
py3, they are unicode strings, not bytestrings.
Show outdated Hide outdated pynest/nest/raster_plot.py
@@ -121,9 +116,21 @@ def from_file(fname, **kwargs):
def from_file_pandas(fname, **kwargs):
"""Use pandas."""
if isinstance(fname, (list, tuple)):
if isinstance(fname, str):

This comment has been minimized.

@apeyser

apeyser Jul 26, 2016

Contributor

Ahh -- I think my suggestion was unclear. I just meant that if you do

if isinstance(fname, str):
    fname = [fname]

if isinstance(fname, (list, tuple)):
    ....

then you could eliminate lines 121-125 here, which are a duplication of 134-146

@apeyser

apeyser Jul 26, 2016

Contributor

Ahh -- I think my suggestion was unclear. I just meant that if you do

if isinstance(fname, str):
    fname = [fname]

if isinstance(fname, (list, tuple)):
    ....

then you could eliminate lines 121-125 here, which are a duplication of 134-146

Show outdated Hide outdated pynest/nest/raster_plot.py
if not isinstance(f, str):
print >> sys.stderr, 'List entry is not a python2 string'
return None
if data is None:
dataFrame = pandas.read_csv(

This comment has been minimized.

@apeyser

apeyser Jul 26, 2016

Contributor

And shouldn't this be:

for f in fname:
   dataFrame = panda.read_csv(...)
   newdata = dataFrame.values
   if data is None: data = newdata
   else: data = numpy.concatenate(...)

Currently it looks to me like this would only read the first fname.

@apeyser

apeyser Jul 26, 2016

Contributor

And shouldn't this be:

for f in fname:
   dataFrame = panda.read_csv(...)
   newdata = dataFrame.values
   if data is None: data = newdata
   else: data = numpy.concatenate(...)

Currently it looks to me like this would only read the first fname.

@apeyser

This comment has been minimized.

Show comment
Hide comment
@apeyser

apeyser Jul 27, 2016

Contributor

@sanjayankur31
👍 Much more concise!

@heplesser Comments?

Contributor

apeyser commented Jul 27, 2016

@sanjayankur31
👍 Much more concise!

@heplesser Comments?

Show outdated Hide outdated pynest/nest/raster_plot.py
else:
data = numpy.loadtxt(fname)
print >> sys.stderr, 'fname should be of str/list(str)/tuple(str).'

This comment has been minimized.

@heplesser

heplesser Aug 2, 2016

Contributor

Didn't now this redirection was possible, in Python. But I am rather sure this line won't work in Python 3, or?

@heplesser

heplesser Aug 2, 2016

Contributor

Didn't now this redirection was possible, in Python. But I am rather sure this line won't work in Python 3, or?

Show outdated Hide outdated pynest/nest/raster_plot.py
else:
data = numpy.loadtxt(fname)
print >> sys.stderr, 'fname should be of str/list(str)/tuple(str).'
return None

This comment has been minimized.

@heplesser

heplesser Aug 2, 2016

Contributor

This return as superfluous, I think.

@heplesser

heplesser Aug 2, 2016

Contributor

This return as superfluous, I think.

fname : str
Name of file
fname : str or tuple(str) or list(str)
File name or list of file names

This comment has been minimized.

@heplesser

heplesser Aug 2, 2016

Contributor

I think it would be good to document how lists of files are handled. If I understand it right, data from files are concatenated and plotted as if they had been stored in a single file?

@heplesser

heplesser Aug 2, 2016

Contributor

I think it would be good to document how lists of files are handled. If I understand it right, data from files are concatenated and plotted as if they had been stored in a single file?

@heplesser

This comment has been minimized.

Show comment
Hide comment
@heplesser

heplesser Aug 2, 2016

Contributor

@sanjayankur31 Looks quite good to me, I just added some small comments (and deleted some again, I had not realised at first that the loading functions call the plot function).

Contributor

heplesser commented Aug 2, 2016

@sanjayankur31 Looks quite good to me, I just added some small comments (and deleted some again, I had not realised at first that the loading functions call the plot function).

sanjayankur31 added some commits Aug 2, 2016

Show outdated Hide outdated pynest/nest/raster_plot.py
@@ -117,8 +121,7 @@ def from_file(fname, **kwargs):
except ImportError:
from_file_numpy(fname, **kwargs)
else:
print >> sys.stderr, 'fname should be of str/list(str)/tuple(str).'
return None
print('fname should be of str/list(str)/tuple(str).')

This comment has been minimized.

@heplesser

heplesser Aug 3, 2016

Contributor

"fname should be one of ..."

@heplesser

heplesser Aug 3, 2016

Contributor

"fname should be one of ..."

@heplesser

This comment has been minimized.

Show comment
Hide comment
@heplesser

heplesser Aug 3, 2016

Contributor

@sanjayankur31 Just one more little text fix (see in-text comment) and I will merge.

Contributor

heplesser commented Aug 3, 2016

@sanjayankur31 Just one more little text fix (see in-text comment) and I will merge.

@heplesser heplesser merged commit ff3cffb into nest:master Aug 3, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@sanjayankur31 sanjayankur31 deleted the sanjayankur31:use-pandas branch Aug 3, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment