ERFA wrapper tweaks #3173
ERFA wrapper tweaks #3173
Conversation
|
@eteq - this looks very good -- again apologies for merging too soon! (Though at least it kickstarted quite a bit of nice further work...). I particularly like the improved docstrings. Would you want to include @jwoillez's new part exposing constants (mhvk@14a7ab8)? If it is too much hassle, I'm happy to do it after too. |
|
p.s. Since it is more of a bug, you may also want to include mhvk@f7c59ee (of course, only the template one being important). |
|
@mhvk - I included mhvk/astropy@f7c59ee just now because that's pretty straightforward and basically a bug fix. Thanks! I didn't include the constants stuff though, as I think that's a more substantial change, so I think it makes more sense to treat it separately as part of PR #3137 . (Regardless, merging any of this should wait on #3159 or its follow-ons) |
|
OK, no problem. It will be easy to redo #3137 once this is in. |
|
@astrofrog - ok, I think once the auto-generation is in this should be pretty easy to rebase. If there's serious delays to #3166 we can merge this in time for the feature freeze without it, if necessary. |
In py 2.x, unicode_literals makes the generated strings look like u'foo', instead of 'foo', as they do in 3.x
|
@mhvk @astrorog - this is now rebased on #3166 . @mhvk - the order of merging might be important here. I think this is best first because it does a lot of little fiddly things that could be a lot of effort to rebase. But definitely other work will have to rebase on this, because it renames some files (which git is sometimes a bit finiky about). Is there anything other than #3137 that you think will conflict? |
eteq
added
the
Affects-dev
|
Ok, cool, then this should be all set. Something weird is going on with travis, though, I think because of some rebasing hackery I did. I noticed some minor typos anyway, so I'll just push up another commit shortly |
|
@mhvk - are you happy with just merging this if the tests pass, or do you want to look over it first? |
|
@eteq - I had another quick look and think this is all OK, so as far as I'm concerned, go ahead and merge once travis is happy. |
|
build passed, so I'll merge it now. |
4 participants
This contains a variety of changes I was planning to suggest as part of reviewing #3141 (but it got merged before I had a chance to suggest them).
The code approach itself is mostly unaltered, but a number of clean-up items:
cython_generator.pytoerfa_generator.py, because it's no longer just generating cython codeerfa.py/erfa.pyxtocore.pyandcore.pyx(which now compiles to_core). The rational for that is that the nameerfa.pyxonly made sense when it was everything in theerfapackage. Now that it's two different modules, the pure-python file is nominalastropy.erfa.erfa, and double-naming like that leads to confusion. So I took a page fromtime(which hasastropy.time.coreas the main implementation file).from __future__ import unicode_literalsinerfa_generator.py. This is deceptively important: with it in, the generated code has a bunch ofu'foo'-style string literals. These are currently not in master, because it apepars @jwoillez use python 3 to generate the code, which has a different output from using python 2 whenunicode_literalsis in force. This will be less important if we solve Auto-generate erfa.pyx and erfa.py #3159, so if I end up rebasing, this could be removed.Note that all the renaming means this will conflict with other ERFA PRs currently pending. Of special signficance is #3170/#3166 or similar work on auto-generating these files during build. It probably makes sense to wait for those, if they're coming soon, and rebase this on those, because that means I can expunge all of the changes to
core.pyandcore.pyx.cc @jwoillez @astrofrog @mhvk @mdboom