round guide count and P2

[javierggt]

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

• [x] 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]

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]

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]

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

[jeanconn]

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).

[javierggt]

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]

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]

(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]

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]

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.

[jeanconn]

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]

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

[taldcroft]

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]

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]

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]

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]

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]

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]

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

[taldcroft]

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

[jeanconn]

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]

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