Skip to content
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

Add coord_system kwarg to get/apply_sun_pitch_yaw and change default sign #37

Merged
merged 1 commit into from
Feb 13, 2024

Conversation

taldcroft
Copy link
Member

@taldcroft taldcroft commented Feb 12, 2024

Description

The current definition of yaw about the sun line matches ORviewer, but this does not match the engineering convention where +yaw is in the same sense as spacecraft body yaw.

The new default matches the engineering sense: it goes from 0 to 360, and has zero be defined as the ECI z-axis if the Sun is at exactly RA = Dec = 0.

This PR adds a keyword argument coord_system which can take the values "spacecraft" or "ORviewer", where "spacecraft" is the default.

Fixes #30

Interface impacts

Changes the sense of the yaw parameter. Tests for sparkles need update (sot/sparkles#206).

Testing

Unit tests

  • Mac (new unit tests)
(ska3) ➜  ska_sun git:(add-spacecraft-system) git rev-parse HEAD             
3579ca1593a3fafb12d7cd67bec4c1de7cdbba42
(ska3) ➜  ska_sun git:(add-spacecraft-system) pytest
====================================================== test session starts ======================================================
platform darwin -- Python 3.10.8, pytest-7.2.1, pluggy-1.0.0
rootdir: /Users/aldcroft/git, configfile: pytest.ini
plugins: timeout-2.1.0, anyio-3.6.2
collected 19 items                                                                                                              

ska_sun/tests/test_sun.py ...................                                                                             [100%]

====================================================== 19 passed in 4.62s =======================================================

Independent check of unit tests by Jean

  • Linux
(ska3-flight-2024.0rc7) jeanconn-fido> pytest
============================================================= test session starts ==============================================================
platform linux -- Python 3.11.7, pytest-7.4.4, pluggy-1.3.0
rootdir: /proj/sot/ska/jeanproj/git
configfile: pytest.ini
plugins: timeout-2.2.0, anyio-4.2.0
collected 19 items                                                                                                                             

ska_sun/tests/test_sun.py ...................                                                                                            [100%]

=============================================================== warnings summary ===============================================================
../../../../../../fido.real/conda/envs/ska3-flight-2024.0rc7/lib/python3.11/site-packages/ska_helpers/version.py:25
  /fido.real/conda/envs/ska3-flight-2024.0rc7/lib/python3.11/site-packages/ska_helpers/version.py:25: DeprecationWarning: pkg_resources is deprecated as an API. See https://setuptools.pypa.io/en/latest/pkg_resources.html
    from pkg_resources import DistributionNotFound, get_distribution

../../../../../../fido.real/conda/envs/ska3-flight-2024.0rc7/lib/python3.11/site-packages/pkg_resources/__init__.py:2868
  /fido.real/conda/envs/ska3-flight-2024.0rc7/lib/python3.11/site-packages/pkg_resources/__init__.py:2868: DeprecationWarning: Deprecated call to `pkg_resources.declare_namespace('sphinxcontrib')`.
  Implementing implicit namespace packages (as specified in PEP 420) is preferred to `pkg_resources.declare_namespace`. See https://setuptools.pypa.io/en/latest/references/keywords.html#keyword-namespace-packages
    declare_namespace(pkg)

-- Docs: https://docs.pytest.org/en/stable/how-to/capture-warnings.html
======================================================= 19 passed, 2 warnings in 32.61s ========================================================
(ska3-flight-2024.0rc7) jeanconn-fido> git rev-parse HEAD
3579ca1593a3fafb12d7cd67bec4c1de7cdbba42

Functional tests

Plotted the yaw value around the 2024:036 NSM yaw bias maneuver using https://gist.github.com/taldcroft/1caaaa0fcddd15e56a51126c869d04e3:

image

@jeanconn
Copy link
Contributor

We still don't have much data to line up. With regard to a "positive yaw bias of 0.025 deg/s for 30 minutes", I thought maybe a positive bias would move us in the negative yaw direction. But I guess the sign convention could be the "bias that makes it go positive" is positive?

@taldcroft
Copy link
Member Author

I think the language is now unambiguous and clear. There is no ambiguity in what a "positive" number is, and so the "positive yaw bias" refers to the value of +0.025 deg/s that is used as input when creating command loads from an RTS.

I specifically added this text to both of the docstrings to remind our future selves.

@taldcroft
Copy link
Member Author

We still don't have much data to line up.

I'm not sure what that means, but with regards to confirming a sign convention you only need one case.

@jeanconn
Copy link
Contributor

Regarding "with regards to confirming a sign convention you only need one case." sure, I just hadn't been looped in on this case. It sounds like you have the convention from the RTS which is fine. I also wasn't sure what you plotted for yaw to confirm it matches expectations -- I assume that is just derived using the code here.

@jeanconn
Copy link
Contributor

We'll need an update to sparkles test_find_er_catalog to go with this.

@taldcroft
Copy link
Member Author

I added a link to the notebook I used to make the plot in the description and here.

@taldcroft
Copy link
Member Author

For reference, the yaw bias RTS is named A_TIMED_YAWBIAS.RTS. This appears in Slack in #anomaly_response_recovery along with "positive yaw bias of 0.025 deg for 30 mins".

@taldcroft taldcroft changed the title Add coord_system kwarg to get/apply_sun_pitch_yaw Add coord_system kwarg to get/apply_sun_pitch_yaw and change default sign Feb 13, 2024
@taldcroft taldcroft merged commit 2ec26c1 into master Feb 13, 2024
2 checks passed
@taldcroft taldcroft deleted the add-spacecraft-system branch February 13, 2024 13:59
This was referenced Mar 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow specifying get_sun_pitch_yaw coordinate convention and change default
2 participants