skyfielders / python-skyfield

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

Add Tycho2 catalog #909

Closed wheirman closed 10 months ago

brandon-rhodes commented 10 months ago

Thank you for this contribution! Could you add a quick test for Skyfield's test suite, so that the feature will keep working in the future without other folks likely to break it? You could add it right next to the hipparcos test:

https://github.com/skyfielders/python-skyfield/blob/41b44cf2a5d18fa963a5db1bcf27e413865fd3ce/skyfield/tests/test_stars.py#L4

With a passing test this will be ready to merge. Thank you again!

wheirman commented 10 months ago

Could you add a quick test for Skyfield's test suite, so that the feature will keep working in the future without other folks likely to break it?

I added a test. Looks like the hipparcos test includes a full copy of hip_main.dat. tyc_main.dat is 344MB so I don't think including that in the Git repo is a good thing, nor is downloading it on each test. So I included just the first 10 lines as tyc_main_head.dat which should be good enough to test the functionality of the importer.

brandon-rhodes commented 10 months ago

Looks like the hipparcos test includes a full copy of hip_main.dat. tyc_main.dat is 344MB so I don't think including that in the Git repo is a good thing, nor is downloading it on each test. So I included just the first 10 lines…

Excellent idea, thanks! The reason for the full-sized Hipparcos catalog is that Skyfield also tests its documentation, which among other things runs an example script that draws a star chart:

https://rhodesmill.org/skyfield/example-plots.html#drawing-a-finder-chart-for-comet-neowise

So having one full star catalog around in the repository to fuel such examples seemed reasonable, given that its 1.2 MB size (compressed) is rather small compared to the 63 MB DE405 ephemeris that Skyfield's tests use to compare its output to the USNO NOVAS library.

But you are entirely correct that we don't want further full star catalogs in the repository! So, again, thanks for thinking about the storage requirements.

wheirman commented 10 months ago
  1. Does epoch_year = 1991.25 need to be changed to 2000.0?

https://cdsarc.u-strasbg.fr/ftp/cats/I/239/ReadMe mentions Note on RAdeg, DEdeg: right ascension and declination are expressed in degrees for epoch J1991.25 for tyc_main.dat, same as for hip_main.dat so it think epoch_year = 1991.25 is correct?

  1. Have you verified that each of the na_values is actually used in Tycho?

Here is df.info for the full tyc_main.dat. Removing some of the na_values introduced object Dtype for some but it looks fine with na_values as-is.

Index: 1058332 entries,    1    13 1 to 9537   380 1
Data columns (total 8 columns):
 #   Column            Non-Null Count    Dtype
---  ------            --------------    -----
 0   magnitude         1055115 non-null  float64
 1   ra_degrees        1058332 non-null  float64
 2   dec_degrees       1058332 non-null  float64
 3   parallax_mas      1035445 non-null  float64
 4   ra_mas_per_year   1035445 non-null  float64
 5   dec_mas_per_year  1035445 non-null  float64
 6   ra_hours          1058332 non-null  float64
 7   epoch_year        1058332 non-null  float64
dtypes: float64(8)
memory usage: 72.7+ MB