nsat / pypredict

Spire port of predict open-source tracking library
GNU General Public License v2.0
64 stars 17 forks source link

Add Python 3 support #33

Closed bjorn-spire closed 4 years ago

bjorn-spire commented 5 years ago

My team only uses Python 3 in our stack, and we need this library for some of the work we're doing.

Sadly I noticed that the test that exists can't run anymore because the C library doesn't allow you to predict a position that's not within 365 days of "now" and I'm not entirely sure how we should reliably address this.

To figure out whether my changes worked or not I tested it by making the following script check_prediction.py:

from pprint import pprint

import predict

# Fetched from http://tle.spire.com/40044
TLE = """0 LEMUR 1
1 40044U 14033AL  18331.86303513 +.00000155 +00000-0 +30474-4 0  9991
2 40044 097.7827 202.5579 0060032 033.9030 326.5994 14.74030059238298"""
QTH = (37.7727, 122.4070, 25)

tle = predict.massage_tle(TLE)
qth = predict.massage_qth(QTH)

time_to_predict = 1543390456.98134  # 2018-11-28T15:34:16.981340Z

pprint(predict.observe(tle, qth, at=time_to_predict))

I then verified that it's behaving the same way by running this script in these permutations:

And finally confirming that it's working:

$ diff -u original-py27 ported-py27
$ diff -u original-py27 ported-py36
--- original-py27   2018-11-28 15:36:17.000000000 +0800
+++ ported-py36 2018-11-28 15:36:00.000000000 +0800
@@ -18,4 +18,4 @@
  'orbital_velocity': 26928.724540608593,
  'slant_range': 10629.375702728315,
  'sunlit': 1,
- 'visibility': 'D'}
+ 'visibility': b'D'}

So it seems like making the C extension compile in Python 3 hasn't broken anything.

If you guys would be okay with taking this in I would also like to:

  1. Bump the version to indicate the new support
  2. Package up manylinux1 wheels for this package
  3. Figure out how to add some kind of automated test that would allow us to know if we accidentally break something. Since the underlying C library has this hard limitation, I'm not sure what to do except making a dynamic test, but that doesn't feel like an option. Overall it would be nice to be able to feed in older TLEs and repredict from there; it would solve a case where we'd like to reprocess data from the beginning of Spire time.

For the packing, I'm going to be doing that anyway now as I need it for our deployment process.

Merging this would also close issue #23

jtrutna commented 5 years ago

@bjorn-spire:

It is indeed a pain that predict.c uses time() and won't run off older TLEs. I was thinking of adding a network dependency (which could also test correctness), by downloading a new TLE and comparing the results against one of the orbit calculating webservices.

Alternatively, since we're using docker so much, just ran across this... https://serverfault.com/a/825073

I don't know what fits into your timeframes, but implementing either of those would be a big boon for pypredict... wink wink.

bjorn-spire commented 5 years ago

spontaneously I think we should just grab the time from the TLE and perform the check against that. If the TLE isn't within that range we could always issue a [warning](https://docs.python.org/3/library/warnings.html] if the TLE is older than 7 days or something reasonable… And keep the hard limit of older than 365 days.

That said, within the scope of what I'm doing right now I don't have time to do this :) But we've talked about reprocessing a lot of old files to get a better canonical reference set of historical data and then this would become relevant again, and I don't mind having the conversation ahead of time so we know where the project think it makes sense to go…

Echelon9 commented 5 years ago

@bjorn-spire your testing of more modern TLEs, closer to wall clock time, looks sound to ensure no regressions.

If you're interested in another orthogonal way of getting comfort that no regressions occurred, you might want to look at this HACK patch (https://github.com/Echelon9/pypredict/commit/2684e06e2fd4626fd335f2b40bc4215a8f4292b2) that I used earlier this year during bringup of continuous integration testing for pypredict.

Note I never finished CI integration up due to competing time demands, but perhaps that patch helps you.