Skip to content

Made _guess avoid a couple cases in which Tables are instantiated and then ignored - #2892

Merged
taldcroft merged 2 commits into
astropy:masterfrom
mdmueller:improve-guessing
Oct 20, 2014
Merged

taldcroft merged 2 commits into
astropy:masterfrom
mdmueller:improve-guessing

Conversation

@mdmueller

Copy link
Copy Markdown
Contributor

It occurred to me that, in addition to adding a weakref to Table and thinking about @taldcroft's ideas in #2881, one immediate way to avoid overhead in io.ascii guessing is to avoid creating a Table if it will later be thrown out. For example, _apply_include_exclude_names checks to see if column names are valid after the Table is instantiated, so I moved the validation code to a method check_column_names which is called after header reading. Also, the issue in #2877 involved a check in _guess to see if len(dat.colnames) <= 1, which can also be moved to the reading code before a Table is created. Not sure if adding an attribute guessing to BaseReader is the right way to go about it, but anyway I'm curious to see what others think.

@embray

embray commented Aug 26, 2014

Copy link
Copy Markdown
Member

Haven't had a close look at this yet, but would it supersede #2880?

@mdmueller

Copy link
Copy Markdown
Contributor Author

Oh yeah, I guess it would. I suppose either approach works, although it's probably optimal to just avoid instantiation altogether. That's a separate issue from the _apply_include_exclude_names fix anyway, so if we go with #2880 instead I could just reduce this PR to only that change.

@taldcroft

Copy link
Copy Markdown
Member

This approach looks good to me. I'll look at the code more closely later, but on first inspection this approach is even better than the advertised "not instantiating a table". In fact it often avoids even reading the data for the table if the header doesn't parse into acceptable columns. Previously if you had a long CSV file then the basic reader would read the whole thing as a single column table with column name "a,b,c" and string data values like "1,2,3". From that perspective this is short-circuiting in a very good way (assuming my initial understanding of the code changes is correct).

@mdmueller

Copy link
Copy Markdown
Contributor Author

True, and that's definitely an advantage of this approach. Of course this still doesn't get around the fact that the data still has to be read and split into lines (although not split into fields) for each reader...now that I think about it, this can be easily avoided by reading the (potential) file in _guess before going through each reader. I think I'll wait until #2716 is merged before opening a PR for that, just to avoid rebasing.

@astrofrog

Copy link
Copy Markdown
Member

@AmrAS1 @taldcroft - what is the status of this PR?

@mdmueller

Copy link
Copy Markdown
Contributor Author

I don't have any more changes in mind, but maybe @taldcroft is interested in further review?

@astrofrog

Copy link
Copy Markdown
Member

@AmrAS1 - ok - btw, it looks like this needs rebasing

@taldcroft

Copy link
Copy Markdown
Member

I'm OK with this, but it looks to be failing a couple of io.ascii tests including test_guess_with_format_arg.

@mdmueller

Copy link
Copy Markdown
Contributor Author

Oops, I made a mistake while rebasing. This should be fixed now--waiting on Travis.

@mdmueller

Copy link
Copy Markdown
Contributor Author

Ah, I see...there was an issue with the fast reader because FastBasic wasn't doing any strict_names/guessing checks. This should be fixed now--waiting on Travis again.

@taldcroft taldcroft added this to the v1.0.0 milestone Oct 20, 2014
taldcroft added a commit that referenced this pull request Oct 20, 2014
Made `_guess` avoid a couple cases in which Tables are instantiated and then ignored
@taldcroft
taldcroft merged commit 30a6279 into astropy:master Oct 20, 2014
@mdmueller
mdmueller deleted the improve-guessing branch October 21, 2014 00:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants