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

Remove old ok-no-starcat stuff #367

Merged
merged 1 commit into from
Jan 13, 2021
Merged

Remove old ok-no-starcat stuff #367

merged 1 commit into from
Jan 13, 2021

Conversation

jeanconn
Copy link
Contributor

@jeanconn jeanconn commented Jan 10, 2021

Description

Remove old ok-no-starcat stuff

This code was meant for SIR ERs or dark cals, and isn't working on modern undercovers and other anyway. Might as well just get a bunch of red warnings on an undercover etc and remove the cruft.

Testing

  • Passes unit tests on MacOS, linux, Windows (at least one required)
    • No unit tests
  • Functional testing
    • Confirmed an old dark cal week (DEC1115A) just shows red warns for the dark cals

Fixes #

@jeanconn jeanconn changed the title Remove old ok-no-starcheck stuff Remove old ok-no-starcat stuff Jan 10, 2021
@taldcroft
Copy link
Member

I haven't looked at the code changes in detail, but I'm a little worried on principle. It sounds like this is going to introduce spurious red warnings where the code is currently suppressing them? Maybe I have that wrong, but I hope we can (to the extent practical) avoid issuing red warnings that we "just know" to ignore.

@jeanconn
Copy link
Contributor Author

Not quite. The code is already issuing somewhat spurious red warnings (on the 62??? part of an undercover). This code supported a special set of OFLSids we no longer use to stop red warnings on SIR (IIRC) and dark cals.

If we wanted to try to cut the red warnings from undercovers and gyro holds based on OFLSid, we could extend this code, but in general, I think it was and is a bad idea to use the oflsids this way.

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.

OK, I looked through this and it seems the code changes correspond to always having ok_no_starcat be undefined or False. That seems uncontroversial.

@jeanconn jeanconn merged commit d897c35 into master Jan 13, 2021
@jeanconn jeanconn deleted the no_starcat branch January 13, 2021 13:42
@javierggt javierggt mentioned this pull request Jul 2, 2021
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