round guide count and P2 #378

javierggt · 2021-11-12T14:37:10Z

Description

This PR adds changes to round P2 and guide count so we do not get warnings due to numerical errors (like we got in NOV1521A.

Testing

Functional testing. I ran this version on the NOV1521A loads. The result is here, and the diff between the two starcheck files is here

jeanconn

Looks good to me. _guide_count looks to be explicitly returning a float, and the P2 from proseco_probs is explicitly a float, so the sprintf behavior should round it. In the absence of unit tests I generally just try to find weeks that show what we want for functional tests.

jeanconn · 2021-11-12T14:51:54Z

So the NOV1521A functional test shows that the bright guide count is working. I figure we probably also want functional test coverage of the OR guide count and P2, or could let them go...

javierggt · 2021-11-12T15:01:42Z

What do you mean by "functional test coverage of the OR guide count and P2"?

Wouldn't that show up in the same output? I posted a diff file to make it easier.

javierggt · 2021-11-12T15:02:59Z

the OR guide count is already rounded in the output, it seems, so there is no associated diff.

jeanconn · 2021-11-12T15:06:11Z

Right, and for the P2 it seems like we should probably update the format string for the print as well (unless the zeros are now reassuring).

taldcroft

Overall the use of sprintf to do rounding and turn the value into a string really seems horrible. But this is perl and after a bit of googling that does seem to be the idiom. So 👍 on this. The question is just about functional testing and interface changes.

And speaking of which, is this a good time to include the guide count in the starcheck per-obsid report instead of just in the summary up top?.

taldcroft · 2021-11-12T15:30:13Z

starcheck/src/lib/Ska/Starcheck/Obsid.pm

@@ -2269,7 +2269,7 @@ sub print_report {
 	my $bad_FOM = $self->{figure_of_merit}->{cum_prob_bad};
 	$o .= "$red_font_start" if $bad_FOM;
 	$o .= "Probability of acquiring 2 or fewer stars (10^-x):\t";
-        $o .= substr(sprintf("%.4f", $self->{figure_of_merit}->{P2}), 0, 6) . "\t";
+        $o .= substr(sprintf("%.1f", $self->{figure_of_merit}->{P2}), 0, 3) . "\t";


What is the substr for here? I see the logic in the change but the original code is confusing me.

I am betting this can be removed.

I can imagine something like this to truncate the number if the rounded number takes more digits than expected, but it is also not a very good idea.

this could related to sprintf actually rounding a float and not giving a string as output. When you round, you might not get a number with exact binary representation, and then when you concatenate and print you get 0.9000001 instead of 0.9, so you truncate the string.

(although I think what I just said would be crazy, I doubt that can be, can it?)

As long as I'm trying out perl... I feel like the substr is not helping out here. In the unlikely event that P2 is > 10 or negative, the substr would give a bad result.

perl -e '$a="10.23432"; $b=substr($a, 0, 3); print("$b\n")' 10.

javierggt · 2021-11-12T15:54:24Z

I also do not like the use of sprintf. I could do int(10*$number + 0.5)/10 or something like that, but I also googled... so...

jeanconn · 2021-11-12T16:01:26Z

Agreed. And I commented on the inputs being floats because I seemed to remember that was the "gotcha" with using sprintf this way (I don't think it rounds if they are already strings)... but I didn't test.

taldcroft · 2021-11-12T21:14:48Z

(I don't think it rounds if they are already strings)... but I didn't test.

Looks like it works...

perl -e '$a="1.23432"; $b=sprintf("%.1f", $a); print($b)'
1.2

jeanconn · 2021-11-12T22:52:01Z

I think your example is not super reassuring, but agree that sprintf looks to round correctly even if the thingy is already a string (I suppose that makes sense it needs to cast to print the float with decimal places anyway).

ska3-fido$ perl -e '$a="1.29432"; $b=sprintf("%.1f", $a); print("$b\n");'
1.3

Good perl reminder.

jeanconn · 2021-11-23T16:24:02Z

Wrt "And speaking of which, is this a good time to include the guide count in the starcheck per-obsid report instead of just in the summary up top?." I went ahead and did that in some commits in this PR after Javier's. I also just added the guide count to the per-obsid stuff at the bottom. It turns out that chunk is not parsed by the parser anyway, so seems not a problem/complication.

starcheck/src/lib/Ska/Starcheck/Obsid.pm

jeanconn · 2021-11-23T16:41:47Z

I put updated output at https://icxc.cfa.harvard.edu/aspect/tmp/starcheck-pr378-v2/starcheck.html and https://icxc.cfa.harvard.edu/aspect/tmp/starcheck-pr378-v2/diff.html though I note we run into the little issue that as it gets further from NOV1521, we get more critical warnings on magnitude mismatches as the supplement has updated.

There might be value in adding an option to starcheck to use the mags from previous starcheck outputs. (just disabling the supplement seems not the right thing and keeping the critical warning still seems safe).

taldcroft · 2021-11-23T22:07:36Z

What about outputting the ER guide star count which has been the driver here?

taldcroft · 2021-11-23T22:19:06Z

About >> CRITICAL: [4] Guide sum mag diff from agasc mag 0.01170, this is definitely not critical in the way that it was when the check was first included. I think it can be demoted to caution or info.

jeanconn · 2021-11-23T22:23:45Z

Good point about the ER bright star count. I was missing that tree in this forest. So, do we want that in the two places, the summary at top and the standard matter at the bottom of an obsid (though this one would only be for ERs or we could include the ER bright star count on ERs and the OR guide count for ORs).

jeanconn · 2021-11-23T22:38:25Z

I don't feel strongly about demoting the guide mag diff critical, just because it shouldn't come up in flight products. For test work, demoting the warning doesn't fix the problem.

taldcroft · 2021-11-23T22:45:37Z

I don't think we need to put the ER count in the summary, but I do think we should have both the ER count and the regular count in the detail text for ERs since both are checked (IIRC?).

taldcroft · 2021-11-23T22:47:33Z

I don't feel strongly about demoting the guide mag diff critical, just because it shouldn't come up in flight products. For test work, demoting the warning doesn't fix the problem.

Well at least it would let us pass test products without a Note related to this issue.

jeanconn · 2021-11-26T20:43:33Z

If the note were based on the diff of all warnings... demoting wouldn't really help... but if it is your preference that's fine.

jeanconn · 2021-11-29T18:32:58Z

OK. I added the count of bright guide stars to the ER standard reporting.

taldcroft · 2021-11-30T11:13:51Z

Can you make the latest output available somewhere? The top description should be updated with relevant permalinks for the functional testing / evaluation.

taldcroft

Looks basically OK apart from the one comment, but I'd like to see the output and diff.

starcheck/src/lib/Ska/Starcheck/Obsid.pm

starcheck/src/starcheck.pl

jeanconn · 2021-12-02T18:11:15Z

The output and diff through 8d9e865 are now in https://icxc.cfa.harvard.edu/aspect/tmp/starcheck-pr378-v3/ (functional testing comment at top updated). Still not sure if you want to call count_guide_stars() again for the summary check; could go either way on that.

jeanconn · 2021-12-02T18:24:05Z

And @javierggt I apparently can't ask for your review because it is your PR but...

taldcroft

Looks good to me.

round guide count and P2

fb53f78

jeanconn reviewed Nov 12, 2021

View reviewed changes

modify P2 format string accordingly

caa0981

taldcroft reviewed Nov 12, 2021

View reviewed changes

jeanconn added 3 commits November 19, 2021 21:38

Remove unnecessary substr()

67acb1b

Add an in-the-obsid check for guide count

b4c6e57

Add guide count in per-obsid text

8fad789

jeanconn reviewed Nov 23, 2021

View reviewed changes

starcheck/src/lib/Ska/Starcheck/Obsid.pm Show resolved Hide resolved

jeanconn added 2 commits November 26, 2021 15:40

Add bright count / count 9th to ER report

b462418

Demote agasc mag diff warn to yellow

eb6168b

jeanconn requested a review from taldcroft November 29, 2021 18:33

taldcroft reviewed Nov 30, 2021

View reviewed changes

starcheck/src/lib/Ska/Starcheck/Obsid.pm Show resolved Hide resolved

jeanconn reviewed Nov 30, 2021

View reviewed changes

starcheck/src/starcheck.pl Show resolved Hide resolved

Explicitly use ER guide count limit for ERs

8d9e865

jeanconn requested a review from taldcroft December 2, 2021 18:11

taldcroft approved these changes Dec 2, 2021

View reviewed changes

jeanconn merged commit d52a819 into master Dec 2, 2021

jeanconn deleted the round_bright_count branch December 2, 2021 20:02

javierggt mentioned this pull request Dec 2, 2021

Update sot/starcheck to 13.14.0 sot/skare3#752

Closed

jeanconn mentioned this pull request Dec 3, 2021

2021.15 sot/skare3#743

Merged

javierggt mentioned this pull request May 18, 2022

2022.5 (closed) sot/skare3#850

Closed

javierggt mentioned this pull request Aug 3, 2022

2022.5 sot/skare3#899

Merged

This was referenced Jan 6, 2023

Update sot/starcheck to 14.0.0 sot/skare3#1005

Closed

Update sot/starcheck to 14.0.1 sot/skare3#1006

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

round guide count and P2 #378

round guide count and P2 #378

javierggt commented Nov 12, 2021 •

edited by jeanconn

Loading

jeanconn left a comment

jeanconn commented Nov 12, 2021

javierggt commented Nov 12, 2021

javierggt commented Nov 12, 2021

jeanconn commented Nov 12, 2021 •

edited

Loading

taldcroft left a comment •

edited

Loading

taldcroft Nov 12, 2021

javierggt Nov 12, 2021

javierggt Nov 12, 2021 •

edited

Loading

taldcroft Nov 12, 2021

javierggt commented Nov 12, 2021

jeanconn commented Nov 12, 2021

taldcroft commented Nov 12, 2021

jeanconn commented Nov 12, 2021

jeanconn commented Nov 23, 2021

jeanconn commented Nov 23, 2021

taldcroft commented Nov 23, 2021

taldcroft commented Nov 23, 2021

jeanconn commented Nov 23, 2021

jeanconn commented Nov 23, 2021

taldcroft commented Nov 23, 2021

taldcroft commented Nov 23, 2021

jeanconn commented Nov 26, 2021

jeanconn commented Nov 29, 2021

taldcroft commented Nov 30, 2021

taldcroft left a comment

jeanconn commented Dec 2, 2021

jeanconn commented Dec 2, 2021

taldcroft left a comment

round guide count and P2 #378

round guide count and P2 #378

Conversation

javierggt commented Nov 12, 2021 • edited by jeanconn Loading

Description

Testing

jeanconn left a comment

Choose a reason for hiding this comment

jeanconn commented Nov 12, 2021

javierggt commented Nov 12, 2021

javierggt commented Nov 12, 2021

jeanconn commented Nov 12, 2021 • edited Loading

taldcroft left a comment • edited Loading

Choose a reason for hiding this comment

taldcroft Nov 12, 2021

Choose a reason for hiding this comment

javierggt Nov 12, 2021

Choose a reason for hiding this comment

javierggt Nov 12, 2021 • edited Loading

Choose a reason for hiding this comment

taldcroft Nov 12, 2021

Choose a reason for hiding this comment

javierggt commented Nov 12, 2021

jeanconn commented Nov 12, 2021

taldcroft commented Nov 12, 2021

jeanconn commented Nov 12, 2021

jeanconn commented Nov 23, 2021

jeanconn commented Nov 23, 2021

taldcroft commented Nov 23, 2021

taldcroft commented Nov 23, 2021

jeanconn commented Nov 23, 2021

jeanconn commented Nov 23, 2021

taldcroft commented Nov 23, 2021

taldcroft commented Nov 23, 2021

jeanconn commented Nov 26, 2021

jeanconn commented Nov 29, 2021

taldcroft commented Nov 30, 2021

taldcroft left a comment

Choose a reason for hiding this comment

jeanconn commented Dec 2, 2021

jeanconn commented Dec 2, 2021

taldcroft left a comment

Choose a reason for hiding this comment

javierggt commented Nov 12, 2021 •

edited by jeanconn

Loading

jeanconn commented Nov 12, 2021 •

edited

Loading

taldcroft left a comment •

edited

Loading

javierggt Nov 12, 2021 •

edited

Loading