skyfielders / python-skyfield

Elegant astronomy for Python
MIT License
1.39k stars 209 forks source link

NGC and IC objects integration #823

Open mattiaverga opened 1 year ago

mattiaverga commented 1 year ago

I think it would be nice to have deep sky objects integration in skyfield, like hipparcos database. I have been maintaining OpenNGC database and it's python interface PyOngc for some time and I wonder if you're willing to consider a PR about using PyOngc as data provider for such objects. I think I can reuse most of the code of the Star class by moving it under a parent DistantObject class, then override specific code for Star and DeepSky.

brandon-rhodes commented 1 year ago

I would be happy to add support for deep sky objects, especially if any additional packages are optional and not required for everyone using Skyfield. (Or maybe I could be persuaded, if I took a look at the additional dependency?)

Could you outline what features would stay in Star after DistantObject was factored out, and what additional attributes DeepSky would have, so that I can picture what it will look like before you do all the work of a PR?

mattiaverga commented 1 year ago

From a quick look at the code, most of the Star class code would pass in the DistantObject class, except:

Probably also _observe_from_bcrs() and _compute_vectors() need to be class specific, because not all OpenNGC objects have parallax and velocities defined.

brandon-rhodes commented 1 year ago

I am still catching up following the holidays, but here are some initial thoughts in response:

  1. I would be open to seeing what the __repr__() might look like for a catalog of NGC objects—maybe a list of their designations?
  2. I would prefer to keep catalog interpretation separate from the Skyfield classes themselves. So, for example, the logic for interpreting the Hipparcos catalog lives in its own module, and outputs a catalog-neutral dataframe for the Star class. I think we should do the same thing for the OpenNGC catalog, and give it its own module in the skyfield.data. package, that outputs a generic neutral dataframe for the actual constructor.
  3. Many stars don't have parallax and velocities, but it's fine, because if those are zero then the Star calculations have no effect on the position, since they are adding zeroes to them. My guess would be that NGC and IC objects would be the same: if even a single one of them has proper motion, then the full logic from the Star class needs to be retained, because if I load up a catalog of n objects then even one of them might have a non-zero proper motion.

I'm not yet sure whether the Star class can't already handle NGC and IC objects, once we write up a skyfield.data. module that can read in the catalogs. I guess they have properties that stars don't have—like, for galaxies, their orientation in the sky and arcseconds of width?

mattiaverga commented 1 year ago

Re-thinking about this, I can see three ways it can be implemented:

Method 1 would meet your preference about keeping catalog interpretation separate from the Skyfield classes. The cons are that users will need an active internet connection to download the csv file, like it happens for the Hipparcos file. Also, it would require to convert RA and dec values availabls in the csv file as strings.

Method 2 would overcame the need of downloading the csv file, as data would be available from pyongc package. Some conversion for RA and dec would still be needed, as values are provided in radians.

Method 3 would make data available offline like method 2 and would provide users a more friendly interface. For example, to load a star from hipparcos, now a user needs to:

from skyfield.api import Star, load
from skyfield.data import hipparcos

with load.open(hipparcos.URL) as f:
    df = hipparcos.load_dataframe(f)

barnards_star = Star.from_dataframe(df.loc[87937])

Having Star.from_ongc() would make possible to:

from skyfield.api import Star

crab = Star.from_ongc('NGC1952')

Moreover, thanks to pyongc, one can use several identifier, for example:

Star.from_ongc('NGC1952')
Star.from_ongc('M1')
Star.from_ongc('LBN833')

would all provide access to the same object.

My former proposal of creating DistantObject and Deepsky classes is not necessary, I just thought it would be weird to setup a Star object for a deepsky object, but indeed the Star class would work just fine as it is. RA and dec can be provided to Star class as Angle using radians available from ongc object. The pyongc dependency can be made totally optional by setting it in an extra and the pyongc module import can be made under the from_ongc() method.

In the next days I should have enough spare time to write all the three implementations above (I don't think it should be too hard), then I'll report here and you can choose your preference.

mattiaverga commented 1 year ago

It was easier than expected after all and I already drafted a PR which combine method 2 and 3, since they both serve different purposes (method 3 is easier to load a single object, while with method 2 one can make use of datasets to filter and load multiple objects).

Yet, if you would prefer method 1 to avoid depending on pyongc at all, let me know.