Thanks!

If we do go down this road, it may be worth splitting the erfa.py file into multiple files simply to avoid having a 20000 line file, or we can also consider generting it/them on the fly. At this stage, I think that @eteq or @mdboom should comment on this though, as I'm not sure what the best approach is.

What is the performance difference introduced by this PR?

This is an interesting idea, and certainly an improvement. I still think dropping Cython altogether is going to be the better long term plan. It's not significantly more complex than the Cython approach, but should be much more efficient on all dimensions. I should have a PR for that quite soon and we can compare approaches.

@jwoillez - can you clarify your comment from #3134 :

Now, it seems that compiling erfa itself takes more time than its wrapper.

Did you benchmark this versus the current in-master version and conclude that? Can you give us some specific numbers?

Also, if we do go this route, we really should get auto-generation at build time going like discussed in #3134 ... These 20,000 line edits are quickly going to spiral out of control otherwise.

Sorry about the confusion. Compiling ERFA itself does not take much time. It seems comparable to the compilation time for WCS. The issue is with the 20000 lines of cython generated C code.

I can provide approximate numbers later today.

@mdboom - You might be right. If rewriting all of the cython wrapper with the C api is a bit too much (it would be for me), you could also do so for the cython part left in this PR.

Without pushing one way or another (cython vs C), this PR is ready for review.

For info, compilation times are:

• this PR: 50 s
• master: 175 s

@jwoillez - thanks!

@eteq @jwoillez - can the auto-generation of the pyx file be done in get_extensions inside the setup_package.py file for ERFA? Then we don't have to set up any new infrastructure?

Just to be clear, the compialtion of the actual wrapper file is now actually much faster (a few seconds versus over a minute). The 50s value is for the whole of the astropy build process, right?

@astrofrog Correct for the question above. The wrapper compiles in a few seconds max.

Hmm, this raises an interesting point - auto-generating the wrappers requires jinja2 to be installed, so it would be a dependency for developers. Also, I just realized that I'm not sure if this way of auto-generating the wrappers is going to be suitable for dealing with stable releases. I'll need to think about it more.

Indeed. I might just remove that last commit...

@jwoillez - huh, very nice!

I redid the test and find no significant difference anymore. In any case, someone should double check me.

Just for the record (in case anyone else is wondering) I originally was surprised that the calculation would take this long (0.1s for 1000 calls) but it turns out I get the same timings if I call the C routines in ERFA directly, so this example is completely limited by the execution time of the ERFA C code.

It might be interesting to try the timings above with one of the much simpler ERFA routines, where we might end up being more limited by the time of the wrapper.

Same test with the very simple aper function.

@jwoillez - perfect, thanks! This all looks fine to me, so at this point I'll just leave it up to @eteq to review and merge it (but I suspect this will need to wait until after the US holiday).

@mdboom - if you then have ideas for getting rid of the Cython altogether later on, then we can do this in a separate pull request.

Wonderful!

If you are going to merge this, you may also consider an additional commit (jwoillez@d41c149) that addresses #3137.

I'll merge this now as I think it is clear that it greatly improves compilation speed, does not harm execution speed, and passes all tests.

Please don't merge issues without at least first applying a milestone--thanks!

@embray - apologies, usually I do think about the labels; will try to be better!

No problem--it just really helps me keep things straightened out, and all the better if I don't have to do it for every issue :)

@mhvk - As @astrofrog said, I wanted to review this (and also put the auto-generation back in - this is 1MB of source changes!), but it's a holiday here in the US so I hadn't gotten to it yet. I guess I'll just have to issue another PR with the changes I was going to suggest.

(But very nice work, @jwoillez ! I'm a bit surprised at the performance implications, but good news that we can keep/improve speed while still speeding up compilation!)

@eteq - sorry, I clearly went a bit too fast on this one (perhaps being too keen to get the Time stuff to work!). I thought the auto-generation was better done as a separate PR, but hadn't quite realised the implication of adding the large change to the git history...

@mhvk - well, I don't want to curb your enthusiasm ;)

And after posting that I tried some git tricks to estimate how much it actually takes up in the repo history (which eventually gets compressed). turns out this whole thing ends up 100k compressed, even though it's more like 1.5M of actual changes. I suppose it's because it's a lot of repetitive boilerplate, which means it compresses pretty efficiently...