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

Don't check the OCAT for "hot" ACIS observations if there are no observations (vehicle loads) #48

Merged
merged 3 commits into from
Feb 16, 2022

Conversation

jzuhone
Copy link
Member

@jzuhone jzuhone commented Feb 16, 2022

Description

acis_thermal_check performs a check of FP temperature violations against the OBSIDs in the science load. For vehicle-only loads there are no OBSIDs to check. The new-ish fetch_ocat_data function queries the OCAT with a list of OBSIDs, to check for observations that can go to -109 C because of a low number of expected counts.

For vehicle-only loads, the list of OBSIDs passed to fetch_ocat_data is empty, but instead of handling this properly, the function queries the OCAT and returns garbage, and BrokenPipe errors are returned to screen.

acis_thermal_check appears to proceed normally after this, but to avoid any issue this PR changes the behavior so that the OCAT is not checked if the OBSID list is empty.

Testing

  • Passes unit tests on MacOS, linux, Windows (at least one required)
  • Functional testing

@jzuhone jzuhone requested review from Gregg140 and jazan12 February 16, 2022 15:20
Copy link

@jazan12 jazan12 left a comment

Choose a reason for hiding this comment

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

Hard to argue with this one.

Copy link
Contributor

@Gregg140 Gregg140 left a comment

Choose a reason for hiding this comment

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

Good structured programming/Software Engineering technique frowns upon multiple exits from a function or a method. I don't like/use multiple myself, but it's legal Python so I approve the change.

@jzuhone
Copy link
Member Author

jzuhone commented Feb 16, 2022

@Gregg140 fe4c78d 😉

Copy link
Contributor

@Gregg140 Gregg140 left a comment

Choose a reason for hiding this comment

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

I approve the update.

@jzuhone
Copy link
Member Author

jzuhone commented Feb 16, 2022

@Gregg140 did it fix the issues on your end? I shouldn't merge until I know that for sure (I'm also waiting on my regression tests to finish).

@jzuhone jzuhone merged commit 1033e9a into acisops:master Feb 16, 2022
@javierggt javierggt mentioned this pull request Mar 30, 2022
@javierggt javierggt mentioned this pull request Aug 3, 2022
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.

3 participants