10000 Add monitors and target_offset attributes to ACACatalogTable by taldcroft · Pull Request #328 · sot/proseco · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Add monitors and target_offset attributes to ACACatalogTable #328

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 2 commits into from
Jul 28, 2020
Merged

Add monitors and target_offset attributes to ACACatalogTable #328

merged 2 commits into from
Jul 28, 2020

Conversation

taldcroft
Copy link
Member
@taldcroft taldcroft commented May 15, 2020 •
edited
Loading

Description

This goes along with sot/sparkles#106 and partially addresss #305.

Testing

  • Passes unit tests on MacOS
  • Functional testing

@taldcroft
Copy link
Member Author

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

@jskrist
Copy link
Member
jskrist commented May 18, 2020

@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
Copy link
Member Author

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 sot/sparkles#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
Copy link
Member
jskrist commented May 19, 2020

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
Copy link
Member Author

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
Copy link
Member
jskrist commented May 19, 2020

Thanks for the tip 👍

@taldcroft taldcroft mentioned this pull request Jun 14, 2020
Merged
2 tasks
@taldcroft taldcroft changed the title WIP: Add monitors and target_offset attributes to ACACatalogTable Add monitors and target_offset attributes to ACACatalogTable Jun 15, 2020
@taldcroft taldcroft requested a review from jeanconn June 15, 2020 13:43
@taldcroft
Copy link
Member Author

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

@taldcroft taldcroft requested a review from jskrist June 15, 2020 13:43
@taldcroft
Copy link
Member Author

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

@taldcroft taldcroft merged commit 2b80a5c into master Jul 28, 2020
@taldcroft taldcroft deleted the or-attributes branch July 28, 2020 10:39
@javierggt javierggt mentioned this pull request Nov 17, 2020
Merged
< 8042 /div>
@javierggt javierggt mentioned this pull request Dec 7, 2020
Merged
@javierggt javierggt mentioned this pull request Apr 6, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jeanconn jeanconn Awaiting requested review from jeanconn

@jskrist jskrist Awaiting requested review from jskrist

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@taldcroft @jskrist
0