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 a dynamic focalplane model #207

Merged
merged 6 commits into from Sep 20, 2019
Merged

Use a dynamic focalplane model #207

merged 6 commits into from Sep 20, 2019

Conversation

tskisner
Copy link
Member

This work removes many hard-coded properties and makes use of the newly-added desimodel.io.load_focalplane() to retrieve all positioner properties and the exclusion polygons to use for each positioner.

Opening this PR to more easily track comments and changes. Do not merge yet.

@tskisner
Copy link
Member Author

Ok, this branch is now re-based against master (which was just tagged 1.0.4).

is obtained:

  - The code uses the new desimodule.io.load_focalplane()
    to get all information about the real focalplane device
    information and the exclusion polygon to use for every
    positioner.

  - When using a "fake" focalplane file designed to match
    the information and polygons use by current master, the
    resulting assignment is the same (modulo a small
    difference in a couple positioners that I'm still
    sorting out)

  - This branch can seamlessly load a focalplane state that
    uses the same polygons used by the realtime positioner
    code.

This branch will not be merged until the remaining small unit
test difference is resolved and consistency with master has
been demonstrated on a larger dataset.
…lable targets. This is a hack to achieve consistency with master branch and will be removed shortly.
…rm length and the effective patrol radius used when computing the targets available to each positioner.
…y simple plot function. Express unassigned state in terms of theta/phi angles, since X/Y center is not always reachable. Comment out most unit tests while debugging. [ci skip]
…t. Also remove assumption that positioners can physically reach all targets within the patrol radius (which is not true with variable arm lengths).
@tskisner
Copy link
Member Author

I believe this branch is ready to merge. This code currently requires a recent version of desimodel (>= 0.9.2). In order to use the lastest hardware model, you also need the current trunk of the desimodel data svn. Desimodel svn now includes two hardware models (at least one more will be added in the coming weeks). The run time passed to fba_run (default is current time) determines which hardware model is selected. To use the old model, select a date prior to 2019-09-16. Here is a comparison of the old and new focalplanes showing the positioners at the zero point of their theta range and at the zero angle of their phi range plus PI. Also plotted are the default exclusion polygons:

test_focalplane_compare.pdf

In addition to the unit tests, I have run this on about 300 tiles of DR7.1 comparing the output of master with this branch using the new hardware model. In that comparison there are noticeably fewer science targets assigned per petal with this new hardware model. This is likely due to:

  1. The realistic exclusion polygons are larger than the historic ones used originally in fiberassign.
  2. Realistic positioners may not be able to reach the very center of the patrol disc due to different arm lengths.

The new realism here is just something we have to deal with. It remains to be seen if the assignment efficiency can be improved as part of the work on #176, #179, #183, and #184.

@sbailey
Copy link
Contributor

sbailey commented Sep 20, 2019

@Srheft has independently run this (thanks!) and verified that it doesn't fundamentally break things, though there are slightly fewer science assignments due to the increased exclusion zones causing more collisions to avoid (oh well, that is reality). Merging now.

@sbailey sbailey merged commit 1fc4ada into master Sep 20, 2019
@sbailey sbailey deleted the hwstate branch September 20, 2019 17:53
@sbailey
Copy link
Contributor

sbailey commented Sep 20, 2019

@tskisner please update doc/changes.rst in master with a summary of the changes in this PR. Thanks.

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