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

Change assignment of leftover fibers #321

Merged
merged 2 commits into from Apr 14, 2021
Merged

Change assignment of leftover fibers #321

merged 2 commits into from Apr 14, 2021

Conversation

tskisner
Copy link
Member

@tskisner tskisner commented Apr 1, 2021

After assigning science targets which have remaining observations, and then ensuring that the per-petal quota of standards and sky are met, there may be some unassigned fibers. These unassigned fibers are then assigned to science, standards, and sky if possible.

In the current main branch, science targets with less than one remaining obs are never considered for assignment, even in this "last ditch" effort to assign unused fibers.

This PR changes that behavior to allow assignment to science targets with less than one obs remaining during this final attempt to assign the leftover positioners.

targets with zero obs remaining.  Also remove some stale debug print
statements.
@araichoor
Copy link
Contributor

thanks a lot @tskisner.
I ve tested it (on a rosette pattern of 9 tiles), and it works as expected, great!
I ve only one suggestion: add this "use_zero_obsremain=True" as an argument of assign.run(), run_assign_full() and run_assign_bytile() (with defaulting it to True).
that way, one can always run the fiber assignment with previous behavior, if desired.

@tskisner
Copy link
Member Author

tskisner commented Apr 2, 2021

Thanks for testing @araichoor, I can definitely add a switch to those functions so that the old behavior can be enabled for comparison.

@araichoor
Copy link
Contributor

I realize that I forgot to check if targets which were receiving a fiber before still do after this change: I notice that ~50/350 TARGET_STATE="CALIB" targets now get one observation less.
Overall, that makes ~790 CALIB fibers, against ~840 before: I don t know if that is expected or not.
And I m not if that is an issue or not: that is ~5% less CALIB only, so I suspect it is ok.

@tskisner
Copy link
Member Author

tskisner commented Apr 3, 2021

Hello @araichoor , I am not familiar with this TARGET_STATE property. Internally in fiberassign, each target is categorized into one or more types for purposes of assignment (SCIENCE, STANDARD, SKY, SUPPSKY, and SAFE). This categorization is done using the bits of DESI_TARGET and definitions from the desitarget package. The categories of each target are stored in the bits of the FA_TYPE column.

Are the CALIB targets you are referring to standards, or something else?

@araichoor
Copy link
Contributor

I m not very familiar with those either, but with checking: all the assigned TARGET_STATE="CALIB" targets have FA_TYPE=3, so those should be fiberassign STANDARD, right?
And they have PRIORITY_INIT=-1, which should confirm their standard status.

@geordie666
Copy link

geordie666 commented Apr 3, 2021

Everything in the MTL ledger that is labeled CALIB should be a standard star, yes. But, not everything that is a standard star will be labeled CALIB. This is because some standards are also other target classes (MWS, etc.) and so their state can be governed by a different target with which they have been merged.

In short: Treat TARGET_STATE as informational. It's a useful way for humans to parse how a target's state is changing, but the real information is in PRIORITY, NUMOBS_MORE, etc.

@djschlegel
Copy link

Sorry, I had thought we were already using this behavior. Could we merge this in and start using it?

@tskisner
Copy link
Member Author

Yes, I was going to add the options requested by @araichoor and then merge. Working on it.

@araichoor
Copy link
Contributor

@tskisner : have you had a chance to look at this STANDARD/CALIB minor thing?
is it something expected to possibly happen? if yes, I d say it is not an issue (given that it s only 5% less CALIB fibers).
please let me know if there are further checks I could do.

@tskisner
Copy link
Member Author

Recall that the old behavior for leftover fibers (i.e. unassigned fibers after 10 standards / 40 sky per petal) was:

  1. Assign to science with numobs_more > 0 (this was typically a no-op, since these would have already been assigned. Only a few corner cases here if a fiber was bumped for sky/standard and then freed up a previously unassignable target)
  2. Assign to any standards
  3. Assign to any sky

Now we have changed step (1) for these leftover fibers to be allowed to observe any science target regardless of numobs_more. So I would expect fewer "extra" observations of standards (we should always have 10 per petal though).

…rsubscribing done targets with leftover fibers.
@tskisner
Copy link
Member Author

Ok, merging this now that checks have passed. Next up is timestamps...

@tskisner tskisner merged commit 5d00ecc into master Apr 14, 2021
@tskisner tskisner deleted the obs_zero branch April 14, 2021 05:38
@araichoor
Copy link
Contributor

ok great then, that explains why we get slightly fewer STANDARD fibers.
I ve checked, using FA_TYPE=3 to identify standards: I confirm that with the obs_zero branch the petals where there are less standards are petals where there were >10 standards in the master branch run (except for the last petal of the last pass, where there are 9 standards instead of 10)

with the master branch (col1 = passid , col2-11: nstd per petal):
1 [10 10 9 10 10 10 10 10 10 9]
2 [10 10 9 10 10 10 10 10 10 10]
3 [10 10 9 10 10 10 10 10 10 10]
4 [10 10 10 10 10 10 10 10 10 9]
5 [10 10 10 10 10 10 10 10 10 10]
6 [10 10 10 10 10 10 10 10 10 10]
7 [10 10 15 10 10 10 10 10 10 10]
8 [11 10 21 14 10 10 10 10 10 9]
9 [15 10 23 19 10 12 15 10 10 12]

with the obs_zero branch (col1 = passid , col2-11: nstd per petal):
1 [10 10 9 10 10 10 10 10 10 9]
2 [10 10 9 10 10 10 10 10 10 10]
3 [10 10 9 10 10 10 10 10 10 10]
4 [10 10 10 10 10 10 10 10 10 9]
5 [10 10 10 10 10 10 10 10 10 10]
6 [10 10 10 10 10 10 10 10 10 10]
7 [10 10 10 10 10 10 10 10 10 10]
8 [10 10 10 10 10 10 10 10 10 9]
9 [11 10 11 10 10 10 10 10 10 9]

@tskisner
Copy link
Member Author

Ok, thanks for that investigation. Working now on the UTC fixes in desimodel, and then will make a corresponding PR for fiberassign.

@araichoor
Copy link
Contributor

awesome, thanks a lot!

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

4 participants