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

Trimming target file using desimodel.footprint.find_points_in_tiles() for fiber assignment #99

Closed
Srheft opened this issue Feb 10, 2019 · 7 comments

Comments

@Srheft
Copy link

Srheft commented Feb 10, 2019

@sbailey : this is more of an inquiry than a ticket so please close it if irreleant.

Using desimodel.footprint.find_points_in_tiles() to trim the input targets for fiberassignment, could lead to unassigned fibers on edges of the tiles in low density fields. In other words, passing a not exactly cut-to-foot target file, improves the assignment efficiency on tile edges.

Plots below show the same fiber (3180) that is left unassigned when I only passed the "in-foot" targets that desimodel.footprint.find_points_in_tiles() gives out and gets assigned when I pass a looser region that encompasses the tile. The fiber seems to be able to reach the target so does this mean that the maximum partol radius should be added to the tile radius to allow that target to be considered "in-foot" in find_points_in_tiles() algorithm or there is a reason for the way that the tile radius is determined? there were three unassigned "edge" fibers like this in this tile (among POS fibers so I'm not talking about the ETC fibers). This is from the ALL_SKY assignment but I imagine it can be the case elsewhere as well.

image image

@sbailey
Copy link
Contributor

sbailey commented Feb 12, 2019

For context, desimodel.footprint.find_points_in_tiles() uses desimodel.focalplane.get_tile_radius_deg() for the default radius. This returns the radius of the middle of the outermost positioner, but as you found here that means that there are some targets that are reachable by some positioners that aren't included. This choice was motivated by people doing coverage statistics and not wanting to include too many points that couldn't actually be observed.

I'd be fine with updating desimodel.focalplane.get_tile_radius_deg() to be the furthest radius that any positioner could reach and just accept that that will bring in a superset of targets that actually could be reached. I don't think we should try to update desimodel.footprint.find_points_in_tiles() to do the exact per-positioner coverage calculation which would be much much slower (ok if someone wants to contribute that as a separate function; I have ideas about how that could be done...)

Any strong opinions either way from anyone about whether the tile "radius" should be referring to the maximum possible radius, or the typical radius?

This shows the radius of furthest reach of the positioners along the outer edge of the tile, along with the radius equivalent to what is currently considered to be the tile radius.
image

@kdawson1000
Copy link

I vote that it is updated to be the maximum possible radius to ensure that all targets will lie within reach of a positioner. I agree that this does not need to do the exact per-positioner coverage.

@Srheft
Copy link
Author

Srheft commented Feb 12, 2019

Would be nice to show the tile perimiter in tile plots of the fiber assignment (a fine dashed-line circle superposed on the plots just so that we know if an "edge fiber" is left unassigned, it's not for any reason other than insufficient target density or fiber collision).

And, @sbailey: Agreed. clearly desimodel.focalplane.get_tile_radius_deg() controls the find_points_in_tiles() output so this is more of a "tile radius definition" issue than the find_points_in_tiles() issue. This ticket just shows how we reached to the conclusion about [possibly] changing the tile radius definition.

@tskisner
Copy link
Member

Another note: In fiber assignment, the tile radius (which is used in the tree lookup to find available targets for each tile) is currently hard-coded to 1.65 degrees. This and all of the hardware properties will be reviewed after the merge when updating the positioner geometry, etc. That would be a good time to get this value from some other place like desimodel.

@tskisner
Copy link
Member

@Srheft
Copy link
Author

Srheft commented Feb 12, 2019

@tskisner

In fiber assignment, the tile radius (which is used in the tree lookup to find available targets for each tile) is currently hard-coded to 1.65 degrees.

The source of difference between hard-coded tile radius in hardware and what I see below is not clear to me:

import desimodel.focalplane
desimodel.focalplane.get_tile_radius_deg()
1.6057735024174122

@sbailey
Copy link
Contributor

sbailey commented Feb 21, 2019

default tile radius updated in PR #102; closing this ticket.

@sbailey sbailey closed this as completed Feb 21, 2019
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

No branches or pull requests

4 participants