Load deepsky objects from pyongc #824
Conversation
setup.py
Outdated
| @@ -60,4 +60,7 @@ def make_distribution(self): | |||
| 'numpy', | |||
| 'sgp4>=2.2', | |||
| ], | |||
| extras_require={ | |||
There was a problem hiding this comment.
For now let's avoid changing setup.py. If we tell users in the documentation that they need to install Pandas and pyongc, then I think that will be enough to get them going if their project needs NGC objects.
skyfield/starlib.py
Outdated
| @@ -106,6 +106,18 @@ def from_dataframe(cls, df): | |||
| epoch=epoch, | |||
| ) | |||
|
|
|||
| @classmethod | |||
There was a problem hiding this comment.
Let's avoid adding any new methods to Star, since it will become a very large class if it has to learn details about every kind of external catalog. Instead, it is designed to stay simple, and every catalog simply needs to format its Pandas dataframe for the from_dataframe() method, and then the star-catalog details remain over in the code specific to those catalogs, instead of needing updates to the class itself, which ideally shouldn't even need to know that different catalogs exist.
| star = api.Star.from_dataframe(df) | ||
| assert repr(star) == 'Star(ra shape=9933, dec shape=9933, ra_mas_per_year shape=9933, dec_mas_per_year shape=9933, parallax_mas shape=9933, epoch shape=9933)' | ||
|
|
||
| def test_dataframe_from_ongc(): |
There was a problem hiding this comment.
Thank you for including tests! Many developers don't think to write them, and it's very helpful.
| df = ongc.load_dataframe(ongc_data) | ||
| hercules = api.Star.from_dataframe(df.loc['NGC6205']) | ||
| assert repr(hercules) == ( | ||
| 'Star(ra=250.42345833333337, dec=36.46130555555556, ra_mas_per_year=-3.18, ' |
There was a problem hiding this comment.
Normally I would object to a repr() since even very small changes in how numbers are input and rounded will break the tests, but in this case it's a bit elegant—it lets us very quickly verify that all the essential properties of the star are correct. So let's keep it this way, and see if it ever breaks!
| """Convert a dataframe from pyongc.data for import in skyfield.""" | ||
| try: | ||
| from pandas import read_csv | ||
| except ImportError: |
There was a problem hiding this comment.
You did a good job repeating the patterns in hipparcos.py!
Signed-off-by: Mattia Verga <mattia.verga@tiscali.it>
|
I've pushed the requested changes, let me know if it's now ok. |
Thanks for the update! I am about to travel to a Python event, which might delay my review, but this issue is at the top of my list for when I get back home, so stay tuned—I'll be careful not to merge any other code until I've had the chance to look this over again. 🙂 |
Loading deepsky objects from pyongc is possible in two ways: a simple load using
Star.from_ongc('identifier')or by converting a dataframe from pyongc.data withskyfield.data.ongc.load_dataframe()+Star.from_dataframe()for loading several objects at once.The pyongc dependency is optional (users needs to install skyfield[ongc] extra to pull in pyongc dependency).
Fixes #823