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.

Sign up for GitHub

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

Jump to bottom

More small dither-related changes #277

Merged
merged 9 commits into from
Jan 17, 2019
Merged

More small dither-related changes #277

merged 9 commits into from
Jan 17, 2019

Conversation

jeanconn
Copy link
Contributor

More small dither-related changes

This PR

  1. puts the tiny bugfix to short-circuit dither checks sooner (when no observation start time available to do them).
  2. tracks the maximum guide amplitude in y and z over the observation instead of just the guide amplitudes 8 minutes after maneuver end (should only matter for big dither but...)
  3. adds a green warning for observations that have different acq and guide dither (again should only be interesting for big dither)

@jeanconn jeanconn requested a review from taldcroft January 12, 2019 18:01
@jeanconn
Copy link
Contributor Author

For obsid 20168 this basically looks like this at the top

Dither:  ON Y_amp=64.0  Z_amp= 8.0  Y_period=2647.1  Z_period=1000.0

And this down in the warnings

>> WARNING: Non-standard dither
>> WARNING: Reviewed with ACQ dither Y=20.0 Z=20.0 
>> WARNING: Max Y Z ampl during guide used for checking Y=64.0 Z=20.0 
>> INFO   : Observation passes 'big dither' checks

taldcroft
taldcroft reviewed Jan 13, 2019
View reviewed changes
starcheck/src/lib/Ska/Starcheck/Obsid.pm Outdated Show resolved Hide resolved
starcheck/src/lib/Ska/Starcheck/Obsid.pm Outdated
@@ -707,17 +712,18 @@ sub check_dither {

$self->{dither_acq} = $acq_dither;
$self->{dither_guide} = $guide_dither;
$self->{dither_guide_y_max} = $guide_dither->{ampl_y};
$self->{dither_guide_z_max} = $guide_dither->{ampl_p};
Copy link
Member

Choose a reason for hiding this comment

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

Not sure if this is opening a can of worms, but you should be consistent and use p(itch) / y(aw) everywhere or y(angle) / z(angle) everywhere. Otherwise it gets very confusing. Since existing code seems to be pitch/yaw then that might be the easiest choice. I don't have a strong preference except to use one system.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think "everywhere" is a can of worms, but I'll at least try for check_dither.

starcheck/src/lib/Ska/Starcheck/Obsid.pm Outdated Show resolved Hide resolved
starcheck/src/lib/Ska/Starcheck/Obsid.pm Outdated Show resolved Hide resolved
starcheck/src/lib/Ska/Starcheck/Obsid.pm Outdated Show resolved Hide resolved
starcheck/src/lib/Ska/Starcheck/Obsid.pm Outdated Show resolved Hide resolved
@jeanconn jeanconn force-pushed the more_dither_changes branch 3 times, most recently from 221c7b1 to e6ee4a9 Compare January 14, 2019 21:06
@jeanconn
Copy link
Contributor Author

jeanconn commented Jan 14, 2019 •
edited
Loading

I didn't go crazy with the conversion of y/z to y/p but made some progress on that front and think I've addressed the rest of the comments. I've only test run this against JAN0419T and AUG2018T thus far (was planning to run and review more regression tests outputs on the full 13.0 candidate).

@jeanconn
Copy link
Contributor Author

Is this GTG or do I need to try to remove each line to see if it is no longer necessary?

@jeanconn
Short-circuit dither checks sooner to avoid uninitialized warns
9a7a68a
@jeanconn
Track max guide dither for obs with multiple guide dithers
5b7d82c
@jeanconn
Add warn about different acq dither with dither amplitude
56b4e9b
@jeanconn
Move more of the Obsid dither to y/p from y/z
da11be3
@jeanconn
Update 'not checking' dither warning
218a884
@jeanconn
Set check for defined guide dither on a key in the hash ref
c5736d6
@jeanconn
Set yellow warns when acq or guide dither set to defaults
5605e95
@jeanconn
Move some weird case dither things to info statements
457b82c
@jeanconn
Move dither setting a bit later in check_star_catalog
5dfd54e 
The new warnings are popping up on Dark Cal replicas.  While
a better plan would be to just not call check_star_catalog if there
is no star catalog, we can get rid of the dither warnings by just
hitting the check for MP_STARCAT before setting and checking that
there is reasonable dither assigned to the observation.
@jeanconn jeanconn force-pushed the more_dither_changes branch from e6ee4a9 to 5dfd54e Compare January 17, 2019 00:51
@jeanconn
Copy link
Contributor Author

After reviewing some regression test outputs, I moved the new dither checking at the top check_star_catalog after the check to see if there is actually a star catalog (even though this was already approved). Rerunning regression outputs, but I think this change is non-controversial and will plan to merge after double-checking outputs.

@jeanconn jeanconn merged commit d95ac3b into master Jan 17, 2019
@jeanconn jeanconn deleted the more_dither_changes branch January 17, 2019 01:46
@javierggt javierggt mentioned this pull request Apr 12, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@taldcroft taldcroft taldcroft approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@jeanconn @taldcroft