New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Segmentation fault, and a couple of API questions #8
Comments
Dear @brandon-rhodes, thanks so much for your detailed thoughts. It's great that you're interested in In your example, the problem is with the interface. The from cysgp4 import PyTle, propagate_many
line1 = '1 25544U 98067A 14020.93268519 .00009878 00000-0 18200-3 0 5082'
line2 = '2 25544 51.6498 109.4756 0003572 55.9686 274.8005 15.49815350868473'
tle_strings = ('Foof', line1, line2)
tles = [PyTle(*tle_strings)] # Note: could also be a scalar, propagate_many
doesn't care
result = propagate_many(mjds, tles)
# ---------------------------------------------------------------------------
# RuntimeError Traceback (most recent call last)
# <ipython-input-7-d395a60f817b> in <module>
# ----> 1 result = propagate_many(mjds, tles)
# cysgp4/cysgp4.pyx in cysgp4.cysgp4.propagate_many()
#
# RuntimeError: Error: Satellite decayed The exception is correct, because the TLE is quite outdated for the MJD that was provided: tles[0].epoch
# <PyDateTime: 2014-01-20 22:23:04.000416 UTC>
tles[0].epoch.mjd
# 56677.93268519 Nevertheless, you hit a weak spot. I'll need to add some sanity checks and not just blindly passing the arrays into the C++ constructors. My apologies for the inconvenience. The other questions will need some further thoughts, especially when it comes to optimizing the OpenMP loops. I'll get back to you. Best, |
There is a link to the "Homepage" (which is the repository) on PyPI. I may add some more direct links to the manual, GH issues etc. in the next release. |
Indeed, it could be worth-wile to try to further optimize the Note also, that
If it's technically possible, that'd be fine with me. Not sure, how to do it. Internally, in the C++ library, |
I don't think, it would be worth the effort. Compared to the number-crunching in the SGP4lib, the few if statements are almost a no-op. (This is compiled code!) |
This is exactly the reason, why I invented the Again, in my original solution, I had the SGP4 instances created for each TLE before starting the loops, but that failed horribly in my multi-threaded approach, as different threads could potentially interfere with each other, if they operated on the same SGP4 instance. One could perhaps add a single-threaded function, that does this (would also be interesting for benchmarking), but on modern machines a lot of potential to improve the real runtimes lies in the parallelization, doesn't it? |
Thanks for looking at my example and figuring out what went wrong! Manual type checks are always a bit tedious, without any guarantee that every possible bad value has been checked.
Ah, understood. I was looking for something named "GitHub" or "Repository"; "Homepage" sounded like it went to a website or readthedocs page.
Yes, it's a bit startling how large the "satrec" struct is. I have not yet worked out why all those values need to survive between one call to propagate and the next, when it looks like whole stacks of values are also passed in the very long parameter lists.
Wow, that's a lot of points to produce with only 2k objects! I'll take a look at those notebooks when I have time.
Having spent an hour yesterday learning more about Cython, I recommend against trying — it looks like there's no native mechanism in Cython for "expose this struct via a wrapper", and instead it either has to be done by hand, or else another tool has to be used to generate Cython code. For your use case, it's likely that your current approach is just fine.
You're right, the expense is a very small one. I think I was probably reacting to the aesthetics of calling a big function with lots of arguments that use
No. I probably often respond to software design purely on appearance — "but this is extra work, could it possibly be avoided?" — instead of only reacting to things that when benchmarked truly take lots of extra time. For some reason, I like avoiding extra work even if it doesn't matter. :)
Probably! I haven't had the chance to benchmark it both ways on my little laptop's processor to see what the difference is. Just for fun, I'm going to look today at other options for a thin wrapper around SGP4 that might serve Skyfield without requiring you to shoe-horn extra calling modes or approaches into your nice library — which is an admirable one-step solution for folks wanting to observe satellites from a location! I expect that folks wanting to predict satellite passes will enjoy the fact that you're able to do all the steps of producing an altaz position from inside of a single C routine! |
Feel free to contact me if you need help - at least in the case when you opt into Cython. It can be a bit tenacious, especially, when it comes to wheel building on all three major platforms. I'm also happy to add features into cysgp4 if there is demand. |
Well, I had somehow imaged this would take much less code, but I've just produced a 250-line wrapper around Vallado’s raw unaltered SGP4 C++ code that lets me do simple propagation with one satellite, or propagation of a whole array of satellites and times: https://github.com/brandon-rhodes/python-sgp4/blob/master/extension/wrapper.cpp To keep the C code as simple as possible, it relies on the calling Python code to build the NumPy arrays into which it will dump its output. The result, alas, is slower than your code because it cannot take advantage of multiple cores! On my 4-core laptop, running:
— shows your library taking only 0.012 seconds while mine takes an entire 0.015 seconds. Obviously, making the operation parallel is a big deal. But can I do it without taking on a huge and complicated dependency like Cython? I'm going to step away from the problem for a day or so and ponder. I like the lightweight approach in my little wrapper module, for getting the numbers another library like Skyfield would need. But it does penalize users who would prefer a hefty parallel solution! I'll have to decide whether to make mine more complicated, or stop here and recommend mine for accelerated single-core operation, and yours for situations where they have the capacity to install Cython on their system and want to use multiple cores. |
Wow, that is amazing. For the record, I also ran the Cython's parallelization is not much else than providing a If you don't mind, my last 2 cents: |
PS: If one wanted to parallelize the Vallado code, a deep copy of the |
Ah, good question! Vallado is the author of the official SGP4 implementation at: https://www.celestrak.com/software/vallado-sw.php I had not seen Wagner's implementation, and do not know its history. Reading through it, the variable names and so forth are different enough that it may be an independent implementation rather than just a reformatting of Vallado’s code — does Wagner provide a history of any sort? Vallado updates his code every few years, so here's a repository where one of his fans has created a git history of the versions from the celestrak site: https://github.com/rirze/sgp4-cpp/tree/master
Thanks for the warning about MacOS! I'll also take a look at the numpy broadcast iterator; maybe I could provide something that numpy will accept as a ufunc and let it do the parallel work.
I don't mind at all, thanks for sharing so much of your experience with this code!
Yes, I should have been more precise with my wording: I meant simply that it’s an extra several megabytes on disk once the wheel is uncompressed, versus about 8k for the little wrapper I wrote yesterday, and since an unusual number of satellite tracking folks seem to do doing so on Raspberry Pi devices and other small computers, I have been trying to keep
Agreed. Alas that I won't be able to distribute
Good point, cython is easier to understand! But in this case I could find no equivalent for |
It seems that @dnwrnr is actively maintaining his sgp4 implementation. Perhaps, he could comment? |
It should be possible, I think. After all, one can use all the CPython machinery in Cython, if one is willing to add some boilerplate code (if needed). See e.g. https://gist.github.com/jnothman/9170380, which does something similar? |
Slightly off-topic: is there a license attached to Vallado's C++ library? I couldn't find one in the zip-file, nor in the repository. |
Interesting, I had not seen that gist. It seems to take a type that already exists and is defined, and loops over its members inspecting their names, types, and values. I think that what I need for this "elsetrec" struct, though, is the opposite: the ability to define a Python built-in type and create its "members" array? (The array that, once it existed, this gist would then iterate over.)
The only statement I have found is in the Frequently Asked Questions list at: https://www.celestrak.com/publications/AIAA/2006-6753/faq.php “There is no license associated with the code and you may use it for any purpose—personal or commercial—as you wish. We ask only that you include citations in your documentation and source code to show the source of the code and provide links to the main page, to facilitate communications regarding any questions on the theory or source code.” |
Most code is a copy of the original fortran code that is then modified into whatever language you are using. Some corrections are then applied to fix issues with the original code |
First: I'm excited about the idea of a machine-speed SGP4 implementation that can be called from Python! This will meet an important need for folks finding the pure-Python
sgp4
not fast enough for their use cases, and who are able to install or compile binary packages on their systems.I would like to add a section to Skyfield's documentation showing how to use
cysgp4
to produce the coordinates that Skyfield needs, for users who need to extra performance. My first effort at producing coordinates, alas, crashed the interpreter:The result:
Additionally, here are a few further thoughts that I'll have once
cysgp4
works instead of crashing. I can open issues for them later if it's appropriate:propagate_many()
interface, which will be of the most interest to Skyfield folks, unfortunately does not cache theSatellite
objects it creates but takes raw TLE lines that must be re-created each timepropagate_many()
is called. This could be a noticeable cost to folks needing to get the same array of satellites and do many propagations on them (for, say, animating a live globe). Could the library surface a nativeSatellite
array-of-structs?Satellite
structs, to run a high-performance loop that produced ECI coordiantes from those structs without any of theif-else
logic surrounding topocentric computations.Satellite
object itself looks very heavyweight for what Skyfield needs. It would be faster to inline theSGP4 *thisptr
instead of having separate malloc'd memory, for one thing; and for another, Skyfield would need none of the other attributes or properties of the class. The ideal interface would be (1) a function that takes TLE lines and returns an array of raw inlinedSGP4
structs, and (2) a function that takes the structs and an array of MJD's and produces an array of positions. That would, with the least overhead and levels of indirection, allow massive arrays of satellites to produce coordinates ready for Skyfield. Would you accept a pull request, if I had time to work on one, that provided a simple low-level API of this kind for the use of other astronomy libraries?I hope it's not too inconvenient for me to have attached my first-impression questions to a substantive issue! Again, let me know if you'd like separate issues spun up for them, and thanks for your work on this library!
The text was updated successfully, but these errors were encountered: