search

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

Add monitors and target_offset attributes to ACACatalogTable #328

Closed taldcroft closed 4 years ago

taldcroft commented 4 years ago

Description

This goes along with https://github.com/sot/sparkles/pull/106 and partially addresss #305.

Testing

taldcroft commented 4 years ago

@jskrist - this is an API stub you should be able to work with.

jskrist commented 4 years ago

@taldcroft , I confirmed that this change allows the monitors parameter to be passed to get_aca_catalog() and does not cause any issues. I can also see that the target_offset property exists for the table. From the code change listed in this PR, it doesn't seem like anything is done with these parameters yet, so that's as far as I can go with this for now.

Let me know if you have any specific changes you'd like me to implement on the MATLAB side, or if there is anything you'd like me to test there.

taldcroft commented 4 years ago

The target_offset parameters are actually used in the partner PR https://github.com/sot/sparkles/pull/106/files. (You can search for "target_offset" in the diffs).

Obsid 22333 has a large target offset (120 arcsec), so is a good test case (see also https://github.com/sot/sparkles/issues/138).

OBS,
 ID=22333,TARGET=(143.181390,26.987130,{HD 82443}),DURATION=(15000.000000),
 PRIORITY=5,SI=ACIS-S,GRATING=NONE,SI_MODE=TE_00828,ACA_MODE=DEFAULT,
 TARGET_OFFSET=(-0.033333,0.000000),
 DITHER=(ON,0.002222,0.360000,0.000000,0.002222,0.509100,0.000000),MIN_GUIDE=1,
 MIN_ACQ=1,ROLL=(202.000000,0.000000),PRECEDING=(23168),
 SEGMENT=(1,13800.000000)

With the new code if you supply the correct target offset (including dynamical offset) then the target roll suggestions from sparkles should have the exact ACA attitude as would come from ORviewer. Currently it always assumes zero target offset.

jskrist commented 4 years ago

I see. I have never tried to combine two different PRs in a local branch for testing before. I will be looking into the target_offset in MATLAB today to ensure it contains the dynamic offset you mentioned, and then I may get the PR you mentioned to test it out.

taldcroft commented 4 years ago

About the different PRs, one way is checking out both branches in your local git repos then sys.path.insert(0, path_to_sparkles_git); sys.path.insert(0, path_to_proseco_git) before any imports other than sys. 🤞 Printing their respective __version__ should show the version number ending with the right git commit hash.

jskrist commented 4 years ago

Thanks for the tip 👍

taldcroft commented 4 years ago

Ready for final review, now including some tests of the API.

taldcroft commented 4 years ago

Merging now. If this has a problem in integration then we'll fix.