ACROSS API: Add SAA and EarthLimb constraint classes

[jak574]

Constraint Classes

This PR adds the following constraint classes.

SAAPolygonConstraint

Calculate for a given spacecraft SAA polygon, if the spacecraft is in the SAA at a given time. The constraint class is instantiated with the polygon as an argument, and the object uses __call__ to calculate a bool or array of bools indicating if the spacecraft is inside (True) or outside (False) the SAA. Arguments for are time, an astropyTime object (scalar or array-like) and a pre-calculated ephemeris object (e.g. BurstCubeEphem). time must be a subset of the times for which the ephemeris was calculated.

Constraint classes for BurstCube and Swift are instantiated with the correct SAA polygons for those missions.

EarthLimbConstraint

Calculates if a target is inside an Earth Limb constraint. When instantiating the class, min_angle determines how close to the Earth limb is considered a constraint, setting this to zero means you are calculating for a pure Earth occultation. Arguments for the object are the same as for SAAPolygonConstraint except it adds a required skycoord parameter which calculates the Earth limb constraint for a given SkyCoord object, this can be scalar or array-like.

Tests

The PR also adds pytest tests for each constraint class, validating against pre-calculated values using third party methods, including using skyfield derived numbers and the Swift TOO API.

[lpsinger]

Shapely does 2D Euclidean geometry. Is this code correct for a polygon on the sphere?

[lpsinger]

Please remove all commented out code.

[jak574]

Please remove all commented out code.

You mean in the tests? I thought it was nice to show how the values we tested against were actually created. I've seen it done elsewhere.

[lpsinger]

Please remove all commented out code.

You mean in the tests? I thought it was nice to show how the values we tested against were actually created. I've seen it done elsewhere.

Yes, it is, but if it's commented out, then there's no way to make sure that the commented-out code continues to work. Why not just add a test dependency on skyfield and uncomment that code?

[jak574]

Yes, it is, but if it's commented out, then there's no way to make sure that the commented-out code continues to work. Why not just add a test dependency on skyfield and uncomment that code?

Sure OK.

[swyatt7]

I have a mild concern. How does the web-api handle all of the assert statements? If it returns a 500 internal service error (which I suspect it might), then all of these should be changed to raises with a valid message to the user. I do see that you do use raise exceptions in the base/ephem class. I think we should (as a standard) only use raise exceptions in web-api type validation.

[lpsinger]

I have a mild concern. How does the web-api handle all of the assert statements? If it returns a 500 internal service error (which I suspect it might), then all of these should be changed to raises with a valid message to the user. I do see that you do use raise exceptions in the base/ephem class. I think we should (as a standard) only use raise exceptions in web-api type validation.

One usually use assertions to check for preconditions or postconditions that must be true if the code is functioning correctly. As such a failed assertion should result in an opaque 500 internal service error. However we should not use assertions to check for foreseeable and correctable error conditions.

Take the following example from this PR:

    if time.isscalar:
        # Check that the time is within the ephemeris range, this should never
        # happen as the ephemeris is generated based on this range.
        assert (
            time >= ephem.begin and time <= ephem.end
        ), "Time outside of ephemeris of range"

If this condition must hold because it is a precondition and the code that calls it ensures that it is so, then an assertion is appropriate. If on the other hand this is checking that input provided by the user is in the required range, then no, an assertion is not appropriate --- 400 Bad Request would be a better fit.

[swyatt7]

I have a mild concern. How does the web-api handle all of the assert statements? If it returns a 500 internal service error (which I suspect it might), then all of these should be changed to raises with a valid message to the user. I do see that you do use raise exceptions in the base/ephem class. I think we should (as a standard) only use raise exceptions in web-api type validation.

One usually use assertions to check for preconditions or postconditions that must be true if the code is functioning correctly. As such a failed assertion should result in an opaque 500 internal service error. However we should not use assertions to check for foreseeable and correctable error conditions.

From a user experience perspective, opaque 500 ISE's are never a good time. And will result in more "why is my code not working emails". I don't think there is any harm in raising an exception letting the user know why their parameters were entered incorrectly from the api.

[jak574]

I have a mild concern. How does the web-api handle all of the assert statements? If it returns a 500 internal service error (which I suspect it might), then all of these should be changed to raises with a valid message to the user. I do see that you do use raise exceptions in the base/ephem class. I think we should (as a standard) only use raise exceptions in web-api type validation.

I will never put in an assertion that I expect to ever be triggered except by a coding error. Where I use them, I'm using them because they prove to mypy that something is what it says it is. They should never be able to be triggered by user inputs. If they can be, then we should remove them and replace with raise HTTPException.

[jak574]

I have a mild concern. How does the web-api handle all of the assert statements? If it returns a 500 internal service error (which I suspect it might), then all of these should be changed to raises with a valid message to the user. I do see that you do use raise exceptions in the base/ephem class. I think we should (as a standard) only use raise exceptions in web-api type validation.

One usually use assertions to check for preconditions or postconditions that must be true if the code is functioning correctly. As such a failed assertion should result in an opaque 500 internal service error. However we should not use assertions to check for foreseeable and correctable error conditions.

Take the following example from this PR:

    if time.isscalar:
        # Check that the time is within the ephemeris range, this should never
        # happen as the ephemeris is generated based on this range.
        assert (
            time >= ephem.begin and time <= ephem.end
        ), "Time outside of ephemeris of range"

If this condition must hold because it is a precondition and the code that calls it ensures that it is so, then an assertion is appropriate. If on the other hand this is checking that input provided by the user is in the required range, then no, an assertion is not appropriate --- 400 Bad Request would be a better fit.

This is an assertion because it should never be true. The user provides the input date rate, this generates an ephemeris that covers that range, therefore the ephemeris should always be valid, unless a bug has been introduced to the code.

[jak574]

Please remove all commented out code.

Fixed in 0ddbd3f.

I also add PR #1914 that removes the commented out code for the Ephemeris unit tests.

[jak574]

From a user experience perspective, opaque 500 ISE's are never a good time. And will result in more "why is my code not working emails". I don't think there is any harm in raising an exception letting the user know why their parameters were entered incorrectly from the api.

I agree, 500s should only ever be raised due to the code not functioning properly (i.e. something is fundamentally broken) and never as a result of wrong user input.

I believe I have tried to raise 400 range exceptions wherever there's an issue with inputs. If you find anywhere where this is not the case, it needs to be fixed.

asserts are there for mypy and bug checking purposes only. They should never be triggered by user inputs.

[swyatt7]

Alright my concerns are alleviated. We are OK with the shapely implementation. I'll approve.