Ground Station Observations are Inaccurate
bretcope opened this issue · 6 comments
I noticed the results of station.Observe(sat, start, end, delta) are both inaccurate and inconsistent. When I run the exact same calculation a few seconds later, I get different results.
I tracked the issue down to the way Coordinate.Observe() is being called in a few places. The time parameter is being omitted and it defaults to DateTime.UtcNow, which produces the wrong value.
If I modify the code to use the correct time (the time of the actual observation I'm trying to calculate), it gives me very accurate pass predictions.
I would create a pull request, but I'm not sure exactly what approach you want to take. The places I noticed Coordinate.Observe being called incorrectly are in GroundStation.cs:
GroundStation.Observe Line 66:
var azEl = eciLocation.Observe(posEci);
GroundStation.Observe Line 98:
return eciLocation.Observe(posEci);
GroundStation.IsVisible Line 114:
var aer = Location.Observe(pos);
The first two instances have easy access to the correct time, but IsVisible takes a Coordinate instead of an EciCoordinate, so there's no direct access to the Time property. The parameter type could be changed or you could do a little pattern matching:
pos is EciCoordinate eci ? (DateTime?)eci.Time : null
However, it might be a better catch-all solution to modify Coordinate.Observe to check if either this or to are EciCoordinate, and if so, use their time as the default time instead of UtcNow. I think that's the behavior a typical user would expect.
However, you should also consider what to do if both this and to are Eci and their Time properties are not equal and the time parameter is null. It might be best to throw an exception in that case. Up to you. Just wanted to share my thoughts.
I think that approach could work. I'm hesitant to add special cases to IsVisible because then any future coordinate systems which have specific time requirements would also need entries there. However, I don't foresee adding any more coordinate systems so this might be a good solution until it becomes a problem.
This problem also raises another question, that of the validity of the reframing of an ECI coordinate at a different time simply by passing a new time (EciCoordinate.ToEci, line 123):
return dt == Time ? this : new EciCoordinate(dt, Position, Velocity);
it might be better here also to, if the time we're passing to the conversion isn't the time reported by the original ECI coordinate, to convert this into a geodedic coordinate and convert it back into an ECI coordinate at the specified time.
This might also fix the underlying problem for this issue, because (I believe) then ECI coordinates would be properly converted into the representation at the given time when observing them. See if #8's branch is a valid solution.
Offtopic, but I really need to write a test suite for this library. I have a set of programs which use this library which all work fine, but it's evident that there are conversions and features I don't use.
Interesting. I hadn't thought of that approach. If it works, I'm happy with it.
Tests are worth it, but time consuming, especially for an open source project. I can totally relate. If you get a framework started, I'd be happy to contribute some tests.
I added an additional, required time parameter to GroundStation.IsVisible and made Coordinate.Observe take the ECI's time at GroundStation.Observe lines 66 and 98 so this should be fixed. I believe the ECI time reframing issue may have been a contributor to this issue (and fixing that may have fixed this issue on its own) but for good measure I made sure the time is passed through the whole method chain.
Fair enough. Thanks for the timely response!
If you could test it and let me know, that would be great. If not, that's completely fine, but I am in the process of moving to college at the moment and won't be able to test it myself until next week. This fix will be included in the next NuGet release regardless.
I will try to get to it tonight or tomorrow.