sot / chandra_aca

Chandra Aspect Camera Tools
https://sot.github.io/chandra_aca
BSD 2-Clause "Simplified" License
0 stars 0 forks source link

Use isclose to match star colors against 1.5 or 0.7 #51

Closed jeanconn closed 6 years ago

jeanconn commented 6 years ago

Use isclose to match star colors really close to 0.700 as bad color and close to 1.5 as requiring the 1p5 model.

Closes #50

jeanconn commented 6 years ago

This should probably be tested both with a starcheck end-to-end test and a aca_lts_eval test.

jeanconn commented 6 years ago

Did the aca_lts_eval integration testing by using this code from my PYTHONPATH and got no errors and see expected decreases in the maximum allowed acq temperatures (due to lower probabilities) in the FEB2618 week for 20765.

Old report data old_chandra_aca_20765

New report data

new_chandra_aca_20765

jeanconn commented 6 years ago

Oops, this is only a fix for 0.7 stars. I missed that there had been problems with 1.5 stars (though I suppose that makes sense). I assume we need a similar fix at

https://github.com/sot/chandra_aca/blob/ba6e3054559db0af4102687210c81683fbcaf1b7/chandra_aca/star_probs.py#L303

to be in full compliance with the comment on today's SSAWG "ACTION: there was a bug in the code (floating point compare of AGASC mag to 1.500 was failing unexpectedly) and it has already been fixed in the ACA LTS eval. Chandra_aca module patch merged to master, needs install."

jeanconn commented 6 years ago

I'll find some more useful test output for 1.5 stars.

jeanconn commented 6 years ago

It looks like maybe the 1.5 color was never really a problem:

ipdb> agasc.get_star(190716896)['COLOR1'] == 0.7
False
ipdb> (agasc.get_star(192291664)['COLOR1']) == 1.5
True

which is I think why I can't find a test that shows the 1.5 changes fix anything. I'm not sure what the issue was referenced in the twiki for 20765 (ah, 20765 did have a 0.7 color issue that was fixed with the first changes).

taldcroft commented 6 years ago

Agreed about 1.5. This makes sense because 1.5 can be represented exactly in floating point. The AGASC uses float32 (IIRC) and so here is the root cause problem.

In [1]: np.float64(1.5) == 1.5
Out[1]: True

In [2]: np.float64(0.7) == 0.7
Out[2]: True

In [3]: np.float32(0.7) == 0.7
Out[3]: False

In [4]: np.float64(1.5) == 1.5
Out[4]: True

And yes, 20765 has a 0.7 star. I edited the TWiki accordingly.

The isclose implementation for both 1.5 and 0.7 should be used nevertheless.

jeanconn commented 6 years ago

@taldcroft Are you OK with the additional changes for color1==1.5 and their testing? (PR has approved status after the 0.7 changes).

jeanconn commented 6 years ago

And while this is a no-op for starcheck, I'll wait on load review approval on installing this.

jeanconn commented 6 years ago

@taldcroft For getting LR approval on this, should I just brief this PR or should I actually tag / cut a new starcheck minor version and call it a starcheck update?

taldcroft commented 6 years ago

Just brief the change. In email announcing this, include all the usual required elements for an update (testing, interface, etc), but it doesn't need a new starcheck version. I don't think this needs the full regression testing.