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

Move allowed_rolldev here from sparkles and update pitch/roll table #19

Merged
merged 1 commit into from
Mar 18, 2022

Conversation

jeanconn
Copy link
Contributor

@jeanconn jeanconn commented Mar 2, 2022

Description

Move allowed_rolldev here from sparkles

Also use updated pitch/roll table. The values come from the update file with a restriction to 17deg starting at pitch 125 and shared with MPCWG in March 2022 (MPCWG/MeetingNotes2022x03x24). That file has signed/positive and negative roll ranges but all the magnitudes are the same for each sign for each row, so reduced the table to the unsigned version.

Testing

  • Passes unit tests on MacOS
  • Functional testing - new simple test added that is hardcoded for the current pitch/roll table.

Pitch / rolldev data

(this includes a point added at pitch 180 defined as 0 deg allowed off-nominal-roll)

pitch_roll

Install test

This was successful.

python setup.py install --user
$ cd ~
$ ipython
import Ska.Sun
print(Ska.Sun.__file__)  # version in ~/.local
Ska.Sun.test('-v')  # pass

@jeanconn jeanconn requested a review from taldcroft March 2, 2022 14:55
Ska/Sun/sun.py Outdated Show resolved Hide resolved
Ska/Sun/sun.py Outdated Show resolved Hide resolved
Ska/Sun/sun.py Outdated Show resolved Hide resolved
Ska/Sun/tests/test_sun.py Show resolved Hide resolved
setup.py Outdated Show resolved Hide resolved
setup.py Show resolved Hide resolved
@taldcroft
Copy link
Member

taldcroft commented Mar 18, 2022

I thought the team had decided to set the start of the FSS restriction at a pitch of 125? This is what is in the file here in Ska.Sun [EDIT] but I have moved this to a slack question.
image

@jeanconn
Copy link
Contributor Author

Good point. I probably should have done this as 3 PRs so I'd have kept better track of file versions.

@taldcroft
Copy link
Member

I don't follow the 3 PRs remark. This PR seems good, though you'll want to squash down to one commit at the end.

@jeanconn
Copy link
Contributor Author

jeanconn commented Mar 18, 2022

For me, it would actually have been 4 PRs. One to move the code here from sparkles. One to update to the expanded roll range data file. One to update to the modified-at-126 file. One to update to the modified-at-125 file. I got confused about which version we had and what we wanted because there was all of this combining-into-on-step process.

@jeanconn jeanconn changed the title Move allowed_rolldev here from sparkles Move allowed_rolldev here from sparkles and update pitch/roll table Mar 18, 2022
Also use updated pitch/roll table from March 2022. That file has
signed/positive and negative roll ranges but all the magnitudes
are the same for each sign for each row, so reduced the table to the
unsigned version.
@taldcroft
Copy link
Member

taldcroft commented Mar 18, 2022

I think each of the 4 PR's you mentioned are more at the level of single commits. I had no trouble following this and your single PR as a reviewer. In the end the first 3 PR's you suggested would be abandoned since we don't want commits of the wrong/intermediate data files in the repo.

Copy link
Member

@taldcroft taldcroft left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

@jeanconn
Copy link
Contributor Author

Thanks for all the help with this one!

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.

2 participants