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

Use q_att_raw for ASPSOL quaternions if available #233

Merged
merged 1 commit into from
Aug 13, 2020
Merged

Conversation

taldcroft
Copy link
Member

Description

Make get_atts and friends work with modern ASPSOL data where the desired column is q_att_raw not q_att.

Testing

  • Passes unit tests on MacOS (only ran asp_l1 tests) [TESTS RUNNING at this point]
  • Functional testing

Via a temporary print statement, confirmed that q_att_raw is being used for obsid 23315 (recent) and q_att is being used for obsid 8008 (old).

@taldcroft taldcroft requested a review from jeanconn August 13, 2020 15:02
Copy link
Contributor

@jeanconn jeanconn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that looks like the right thing. I can't think of any code that is going to be dependent on the "wrong" behavior.

I suppose there are a couple of other problems:

  1. The mica vv code is doing its own version of the CentroidResiduals code (so the mica.vv code was tested with the aimpoint correction pipeline change but the residual code was not)
  2. The residuals code doesn't have modern testing against a recent file.

Neither needs to be addressed in this PR.

@taldcroft taldcroft merged commit b8e577a into master Aug 13, 2020
@taldcroft
Copy link
Member Author

I'm slightly confused about your point 1, just in the sense that the mica VV report for e.g. 23315 looks A-OK. Is that not what you are referring to? When I was searching around in the mica code I ran into that mapping that accounts for the various "raw" versions of columns, so I was under the impression it is all just working.

@taldcroft taldcroft deleted the raw-atts branch August 13, 2020 15:26
@jeanconn
Copy link
Contributor

jeanconn commented Aug 13, 2020

My point was that IIRC the code to do residuals is duplicated. That's not ideal and why I missed this for the centroid dashboard use case.

@javierggt javierggt mentioned this pull request Dec 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants