sot / proseco

Probabilistic star evaluation and catalog optimization
https://sot.github.io/proseco
BSD 3-Clause "New" or "Revised" License
1 stars 0 forks source link

Update get_kwargs_from_starcheck_text for monitor handling #355

Closed jeanconn closed 3 years ago

jeanconn commented 3 years ago

Description

Update get_kwargs_from_starcheck_text for monitor handling.

There's awkwardness in this approach, but I don't see a good way to toss in a monitor star into a catalog without building a more general monitor request in the kwargs.

This will not properly guess guide-from-mon conversions if we move to 8x8 image size for science observations, but I figure we can just take call args from proseco pkls for those observations.

Testing

Extended existing test to get both kinds of monitor windows (guide conversion and plain monitor).

Fixes #353

taldcroft commented 3 years ago

@jeanconn - The original get_monitor function was a case where a simple Python loop is easier to understand than numpy mask selection. I started trying to say how this should be done but in the end I just had to start writing code. Then the scope expanded to work with multiple MON entries in a catalog.

In full disclosure, I found that with a BFM and a MON, I was not able to reproduce that catalog using the existing code (with force_catalog=True). There is something wrong, but this case is unlikely to ever occur in the wild.

jeanconn commented 3 years ago

Sure. If you want to support multiple monitor requests too that's fine.

I note that I included the agasc star lookup pathway because I thought it was a good compromise between using the OR list and getting the low-precision position with the yag zag available in the printed catalog, and I thought it might be more flexible if one actually wants to play with the catalog.

There's a bunch of grey area here where "what is the intent of getting the kwargs". For the "force catalog" option it seems to be to find a way to easily make a proseco catalog that ends up with the same result. For the not-forced-catalog option it is a little more vague. I thought perhaps we didn't need the tool for rigorous regression testing after the monitor PR that was tested without this change.

taldcroft commented 3 years ago

The yang/zang in the catalog is guaranteed to get back the same star so there is no need for supplementary information from the OR. The OR values of RA, Dec are going to be within 0.5 arcsec of the catalog yang/zang, so not a problem.

I thought it might be more flexible if one actually wants to play with the catalog

Can you elaborate? With equivalent monitors input, the catalog that proseco generates will be identical.

jeanconn commented 3 years ago

I'd have to look at it again, but I figure if you have your monitors as a YAGZAG input and change the attitude/roll of the catalog you are going to end up tracking the same yag zag instead of tracking the original intended target. That's what I meant by "play".

jeanconn commented 3 years ago

So I went with the "if there's a star" compromise solution. Seems like another possible valuable one would just be to get ra dec back from yag zag and use that in monitors instead using YAGZAG coords for the monitor.

jeanconn commented 3 years ago

Also not sure what to do with these stuck flake8 checks.

taldcroft commented 3 years ago

I just think having such flexibility in this function is over-design. Using a starcheck output to play around with the attitude for a catalog with monitor windows is not something I would do, instead I would use the pickle. For me this convenience is mostly for easy regression testing, not operational planning. Am I missing something?

jeanconn commented 3 years ago

No, sounds good. As I was saying, I wasn't as sure about the design goals (grey area) because I figured the primary regression task was done.

taldcroft commented 3 years ago

About the flake8 checking, I did the usual of removing the checks and then re-adding them. This initially seemed to have not worked, but now I see it looks good.