Allow file comments with genfromtxt(..., names=True) #351

Closed
wants to merge 2 commits into
from

Projects

None yet

4 participants

@travisbot

This pull request fails (merged 9f45975 into 143fb18).

@khaeru
khaeru commented Jul 12, 2012

Ah. OK, so this change will not work with the following:

# gender age weight
M   21  72.100000
F   35  58.330000
M   33  21.99

(using the table from test_commented_header). But it will work with any of the following:

gender age weight # these are the headers
M   21  72.100000
F   35  58.330000
M   33  21.99
# here is a general file comment
# it is spread over multiple lines
gender age weight
M   21  72.100000
F   35  58.330000
M   33  21.99
# here is a general file comment
# the columns in this table are:
gender age weight
# following this line are the data:
M   21  72.100000
F   35  58.330000
M   33  21.99

etc. If this is an acceptable trade-off, I can rewrite the test.

@njsmith
Member
njsmith commented Jul 13, 2012

So it sounds like there's a behavioural change here, where before we required header names to have a comment marker at the beginning and now we require them not to? Or something like that? Discussions about what behaviour we want belong on numpy-discussion, because lots of people who might be affected don't read PRs, so you should send a note there explaining the issue.

@khaeru
khaeru commented Jul 13, 2012

OK, will do.

@travisbot

This pull request passes (merged 74e071e into 143fb18).

@njsmith njsmith commented on the diff Jul 17, 2012
numpy/lib/npyio.py
@@ -1345,8 +1346,10 @@ def genfromtxt(fname, dtype=float, comments='#', delimiter=None,
try:
while not first_values:
first_line = fhd.next()
- if names is True:
- if comments in first_line:
+ if names is True and comments in first_line:
+ if skip_header == -1:
+ first_line = first_line.split(comments)[0]
+ else:
first_line = asbytes('').join(first_line.split(comments)[1:])
@njsmith
njsmith Jul 17, 2012 NumPy member

FYI -- .split takes a second optional argument. All of these should just become first_line.split(comments, 1), and the join parts can just go away.

@charris
Member
charris commented Apr 3, 2013

Ping the travisbot.

@charris charris closed this Apr 3, 2013
@charris charris reopened this Apr 3, 2013
@charris
Member
charris commented Apr 3, 2013

I think this needs a test.

@njsmith As you noted, this seems to change behavior. I don't recall the discussion about that or if anything was decided.

@njsmith
Member
njsmith commented Apr 3, 2013

A quick skim of that mailing list thread suggests that this change would break several people's code. I didn't read enough to see if a more generally acceptable solution was found.

@charris
Member
charris commented Apr 4, 2013

Sounds to me like it should be closed then. I don't know why travisbot it trying to install this in 2.4, but I've seen that in a few other spots.

@charris
Member
charris commented May 11, 2013

Going to close this.

@charris charris closed this May 11, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment