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

qa-related-phase2 #230

Merged
merged 6 commits into from Oct 17, 2019
Merged

qa-related-phase2 #230

merged 6 commits into from Oct 17, 2019

Conversation

Srheft
Copy link
Contributor

@Srheft Srheft commented Oct 14, 2019

Please don't merge this until further notice; will add one more commit for fiber location match up that despite the updates gives unmatched error.

@sbailey
Copy link
Contributor

sbailey commented Oct 17, 2019

These changes are all fine with me; feel free to merge this whenever you are ready.

@Srheft
Copy link
Contributor Author

Srheft commented Oct 17, 2019

@sbailey
Just a note for documentation that prior to this PR, qa-fiberassign would raise three errors per tile at all times, two of them turned out to be unjusted:

1- 800 fibers with CMX_TARGET=0 ---> this is not an error, SKY fibers do not have dedicated bits in different programs;
2- fiber:location map incorrect ---> turned out to be a fiber ID mismatch between ETC fibers with -1 and POS fibers with ID >5000
3- Fibers assigned to more than 6mm from positioner center ---> this has stayed as an errror and needs further investigation.

I applied the key change between the last focalplane format and the current as 'X' no longer exists; X-> OFFSET_X
and I am not sure the physical meaning of the offset_x is the same as X in the context of defining the radius here:

dx = (fa['FIBERASSIGN_X'][ii] - fiberpos['OFFSET_X'][ii])
dy = (fa['FIBERASSIGN_Y'][ii] - fiberpos['OFFSET_Y'][ii])
r = np.sqrt(dx**2 + dy**2)

This radius is consistently >6mm for all the tiles that I ever processd and just by the low of probability, it doesn't look right to always be true; if it's correct then maybe the patrol radius has to be changed to that higher number if positioners keep reaching to targets outside their patrol radius. I can address this in a ticket [and provide more stats as to what is the calculated 'r' value for the tiles that I processed with qa-fiberassign] if you see fit.

Please merge if you approve of my last commit.

@sbailey
Copy link
Contributor

sbailey commented Oct 17, 2019

@Srheft yes OFFSET_X/Y has the same meaning as X/Y; it's the name that the hardware team used in their upstream files. I'll merge this PR now and Let's chase the radius >6mm separately. I see that you are using desimodel.io.load_focalplane which should be correct. It may have something to do with whether the comparisons are done with or without FIELDROT.

@sbailey sbailey merged commit 2b2effa into master Oct 17, 2019
@sbailey sbailey deleted the qa_related_phase2 branch October 17, 2019 16:17
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.

None yet

2 participants