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.

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

Fix AASTexData py2.6 incompatibility/performance boost #4641

Merged
merged 2 commits into from Feb 29, 2016

Conversation

eteq
Copy link
Member

@eteq eteq commented Feb 26, 2016

This is a slight adjustment to the change introduced in #4561 . It just pre-compiles the regex into an object that is then re-used in the re.sub call. While this is probably a trivial improvement on it's own, it has a much more important effect when backported to v1.1.x and v1.0.x: it fixes a py2.6 incompatibility introcued in #4561. It turns out that the flags keyword is not in re.sub in py2.6. So this PR works around that by using the compiled regex instead of compiling it as part of the re.sub call.

In principal this could be put only in v1.0.x/v1.1.x... But I think it's better to put it in master, as it has no downside and might be a performance improvement. Any opinions on that, @astrofrog or @taldcroft ?

(note that no changelog entry is needed because #4561 has not been in a release yet)

this will probably give a slight performance boost and also restores
py2.6 compatibility (inportant because this line was originally added
in a patch for v1.0.x)
@eteq eteq added io.ascii Bug Affects-dev PRs and issues that do not impact an existing Astropy release labels Feb 26, 2016
@eteq eteq added this to the v1.0.9 milestone Feb 26, 2016
@astrofrog
Copy link
Member

👍 I'm fine with putting this in master!

@taldcroft
Copy link
Member

Looks OK. My normal style would be to call it something like re_final_empty_line and define it as a local variable instead of class attribute. I think compiled regexes are cached so this doesn't have any real performance impact.

@taldcroft
Copy link
Member

Actually should just be re_final_line.

@eteq
Copy link
Member Author

eteq commented Feb 29, 2016

Hmm... I didn't realize that about the caching, @taldcroft - good to know!

One caveat though: https://docs.python.org/3.5/library/re.html#re.compile says this:

The compiled versions of the most recent patterns passed to re.compile() and the module-level matching functions are cached, so programs that use only a few regular expressions at a time needn’t worry about compiling regular expressions.

Doesn't that mean we should do this because astropy certainly uses a lot of different patterns in different places? I guess "most recent" is fairly ambiguous, though...

@eteq
Copy link
Member Author

eteq commented Feb 29, 2016

Actually I answered my own question by looking at re.py in the standard library. Turns out despite that wording in the docs, the cache looks to be just an ordinary dict, so it's not just the "most recent". So I'll change to as @taldcroft suggests, as that reduces the at-import runtime.

@eteq
Copy link
Member Author

eteq commented Feb 29, 2016

OK, change made. This muddies the water a bit in terms of whether it's worth having this in master... But my inclination is to just merge this and backport it for now as it probably now is basically neutral in terms of performance. If we want to remove it in another PR down the road, that's probably fine, as we just need to not backport it to 1.1.x or 1.0.x.

@taldcroft
Copy link
Member

@eteq - I'm fine with this going in master.

@eteq
Copy link
Member Author

eteq commented Feb 29, 2016

Alright, I'll merge this now and backport, and that should restore py2.6 compatibility for the 1.0.x and 1.1.x branches.

eteq added a commit that referenced this pull request Feb 29, 2016
Fix AASTexData py2.6  incompatibility/performance boost
@eteq eteq merged commit c82ebd9 into astropy:master Feb 29, 2016
@eteq eteq deleted the fix-py26-incompat-re branch February 29, 2016 21:27
eteq added a commit that referenced this pull request Feb 29, 2016
Fix AASTexData py2.6  incompatibility/performance boost
eteq added a commit that referenced this pull request Feb 29, 2016
Fix AASTexData py2.6  incompatibility/performance boost
@eteq
Copy link
Member Author

eteq commented Feb 29, 2016

backported to v1.1.x in 36d5394 and v1.0.x in 0b3d439

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Affects-dev PRs and issues that do not impact an existing Astropy release Bug io.ascii
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants