skyfielders / python-skyfield

Elegant astronomy for Python
MIT License
1.41k stars 211 forks source link

Dependency to skyfield too heavy for just TEME-GCRS conversion #31

Closed letmaik closed 8 years ago

letmaik commented 10 years ago

I'm using sgp4 to get TEME coordinates. I could use skyfield to convert these to GCRS. My problem is that this adds a massive dependency to my own library, caused by the de421 dependency of skyfield which is 26MB.

The main use case of skyfield probably requires de421 or similar to do its work, and the satellite ephemeris calculations are a side use case I guess. So removing de421 as dependency and letting the user install such package himself may not be what you want (loosing out-of-the box experience). On the other hand, letting the user have more freedom in choosing the package he wants might be good as well. After all, it's just pip install skyfield de421 or pip install skyfield de423 or similar.

What do you think?

tritium21 commented 9 years ago

I would suggest using setuptools' extras mechanism for this. With it you can do something like pip install skyfield[testing] and it will install all the testing dependencies. I would argue for using this facility for installing the ephemeridies (I hope I spelled that correctly)

brandon-rhodes commented 9 years ago

Yes, it does seem like there should be some way for users to get only the logic from Skyfield without the baggage. I wish, @tritium21, that instead of just extras, there was a way to specify skyfield[alone] or skyfield[bare] that provided less than the normal number of dependencies.

What if I were to provide and maintain a separate skyfield_alone package that had the skyfield package inside of it, but declared zero dependencies? I could make sure that the release process always produces both, and users with special requirements — as you have, @neothemachine — could opt to depend on the “alone” version.

letmaik commented 9 years ago

@brandon-rhodes That sounds a bit too crazy and uncommon I think. Currently skyfield has the following dependencies according to setup.py: de421, jplephem, numpy, requests, sgp4. Can you list which features/parts of skyfield need which dependencies (except numpy because that's a no-brainer for scientific code)? Also, can you specifically say whether de421 is a hard requirement for the relevant feature to work or if it also works with de423 for example.

tritium21 commented 9 years ago

Creating a skyfield meta-package that depends on skyfield-core and on the ephemerides would be the solution used widely in systems like apt. it is not without precedent.

letmaik commented 9 years ago

I just looked at the sources of skyfield and jplephem and how it all works together. Currently skyfield hardcodes the use of de421 in skyfield/api.py. I assume the other de4* packages could be used here as well. Therefore, I think it makes sense to hand this decision over to the user, even if it means that they have to include de421 (or whichever they want) explicitly in their setup.py (or pip install it, respectively). Then this issue would be resolved and I think there is no need to have a split up in skyfield and skyfield-core or similar. What do you think?

brandon-rhodes commented 9 years ago

I am still thinking about the issue. The de421 requirement was so that Skyfield could work and give answers out-of-the-box. Without an ephemeris, there is zero that Skyfield can do from its main API, because almost everything has to start with a planet. I am thinking about @tritium21’s idea of a skyfield-core that sophisticated users could select as their dependency if they did not care about Skyfield not working out of the box, and were happy to set up with another ephemeris instead.

letmaik commented 9 years ago

You still didn't say whether users are allowed to use a different ephemeris than de421. Is that a use case of skyfield or not?

ghost commented 9 years ago

Thought: I believe the VSOP coefficients are shorter and wouldn't be as big a dependency as DE421 (or actually DE430 or DE431 these days). The older VSOP coefficients are fairly small, although VSOP2014 might be too large to make a difference. Additionally, the original VSOP coefficients are based on a different reference plane and center point, which may complicate things.

On Wed, 14 Jan 2015, Maik Riechert wrote:

Date: Wed, 14 Jan 2015 00:16:57 -0800 From: Maik Riechert notifications@github.com Reply-To: brandon-rhodes/python-skyfield <reply+0004c41d677d2934e72a67e4b812acb8fc3ffa2f500245d492cf0000000110cde7f 992a169ce02997092@reply.github.com> To: brandon-rhodes/python-skyfield python-skyfield@noreply.github.com Subject: Re: [python-skyfield] Dependency to skyfield too heavy for just TEME-GCRS conversion (#31)

You still didn't say whether users are allowed to use a different ephemeris than de421. Is that a use case of skyfield or not?


Reply to this email directly or view it on GitHub: https://github.com/brandon-rhodes/python-skyfield/issues/31#issuecomment-69882892

brandon-rhodes commented 9 years ago

@neothemachine — Sorry that I left that question hanging — here is the page in the docs about ephemerides:

http://rhodesmill.org/skyfield/planets.html

I will update the title of that section to say “Planets and Choosing an Ephemeris” so that it is more obvious from the Table of Contents that it talks about ephemerides — because right now it hides the subject by titling it just “The Planets.”

letmaik commented 9 years ago

If I see it right then this all boils down to the skyfield.api module, which is a convenience module and the only piece of code having a dependency on de421. If you split skyfield into skyfield-core and skyfield, then I think skyfield.api in its current state should not be part of skyfield-core because it would not be functional without de421. Of course you can say "so be it" and still include skyfield.api in skyfield-core and perhaps print an error when de421 is not available. But somehow I don't like that because it could be cleaner. After all, skyfield.api is kind of an addon to skyfield-core.

So basically what I'm saying is: you could split skyfield into skyfield-core and skyfield, make skyfield a namespace package, and put skyfield.api into the new skyfield (meta)package. This is the cleanest way in my opinion.

brandon-rhodes commented 8 years ago

You will be happy to learn that, after some reflection on the future of Skyfield, I wound up agreeing with you! As you will see on the project home page, there is no longer any automatic dependency on any ephemeris — users have to be explicit (yay Zen of Python!) and ask for one to be downloaded:

from skyfield.api import load, now
planets = load('de421.bsp')

So you should now be able to use Skyfield on small machines without much memory as long as you just need the SGP4 routines and transforms. Enjoy, and feel free to open any further issues if you have a problem!

letmaik commented 8 years ago

Looks good! I think that's the proper approach, definitely more maintainable, especially for you.