Skip to content
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.

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

Provide target offset to proseco or sparkles #141

Closed
taldcroft opened this issue May 14, 2020 · 7 comments
Closed

Provide target offset to proseco or sparkles #141

taldcroft opened this issue May 14, 2020 · 7 comments
Labels
bug Something isn't working

Comments

@taldcroft
Copy link
Member

I believe the source of the issue for obsid 22333 in #138 is that sparkles is not being made aware of the target offset, so it is rolling around assuming TARGET_OFFSET=(0, 0). For obsid 22333 it is actually TARGET_OFFSET=(-0.033333,0.000000). Information about the offset needs to be provided at some point:

  • When getting the initial catalog with get_aca_catalog(..., target_offset_y=..)
  • When creating the AcaReviewTable object (e.g. aca.get_review_table(target_offset_y=...)
  • When running the review (acar.run_aca_review)

The best would be the first so that the target offset gets into the pickle.

This also relates to #122 to include other information from the OR list so facilitate ACA pre-review (target name would be especially useful).

cc: @jayhead13 @jskrist

@taldcroft taldcroft added the bug Something isn't working label May 14, 2020
@jskrist
Copy link
Member

jskrist commented May 14, 2020

Should the target offset be passed in via two separate keyword args, i.e. target_offset_y and target_offset_z, or as a single list, e.g. target_offset = [target_offset_y, target_offset_z]?

@jeanconn
Copy link
Contributor

jeanconn commented May 14, 2020

Also, I think this relates more to sot/proseco#262 than #122. I thought #122 was about using the OR list file "on the side" to help out until values could be supplied from the ORViewer side.

@jskrist
Copy link
Member

jskrist commented May 14, 2020

@jeanconn, I had forgotten about sot/proseco#262. It seems like there are several issues all related to the same thing here (#138, #141, sot/proseco#262, sot/proseco#304, sot/proseco#305). would it make sense to consolidate them into a single issue, or do they each serve a unique enough purpose?

I see each of these as being special cases of something like "standardize the proseco-MATLAB interface" or "pass relevant data between MATLAB and proseco".

@taldcroft
Copy link
Member Author

👍 I'll try to consolidate. The only one in your list that is not simply a variant of the proseco-MATLAB interface is #138, which is partly due to incomplete information but also relates to a real deficiency in the algorithm.

@jeanconn
Copy link
Contributor

Yes, though per the meeting I was thinking of them really as the two MATLAB issues, the "ORViewer passes useful OR information to proseco" issue (which is basically sot/proseco#262 ) and the "MATLAB should rely entirely on proseco catalog for uplinked catalog values" sot/proseco#304 . I think the others still might deserve issues on this side (using target offsets for roll determination and handling monitor window stuff is separate conceptually) but shouldn't need matlab.

@taldcroft
Copy link
Member Author

Should the target offset be passed in via two separate keyword args, i.e. target_offset_y and target_offset_z, or as a single list, e.g. target_offset = [target_offset_y, target_offset_z]?

Whatever is more convenient. There is precedent for passing dither as a 2-component, so that would be the preference.

@jeanconn
Copy link
Contributor

Resolved in matlab tools 2022_030

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants