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

uses the focalplane radius from DESIMODEL instead of the hardcoded version in fiberassign #453

Merged
merged 2 commits into from Mar 22, 2023

Conversation

forero
Copy link
Member

@forero forero commented Mar 10, 2023

This PR fixes bug #452

@forero
Copy link
Member Author

forero commented Mar 10, 2023

These changes only affect the function targets_in_tiles()

Copy link
Contributor

@dstndstn dstndstn left a comment

Choose a reason for hiding this comment

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

Looks fine to me!

@araichoor
Copy link
Contributor

sorry I notice I never hit "send" on a small message I had when you opened the PR.

  • I apologize, I don t have time right now to carefully look at that
  • though one thing we definitely want to check is that the "default" behavior is not changed (ie it does not change anything for the main survey).

I mention this because I notice that the two radii differ:
with the main branch

>>> from fiberassign.hardware import load_hardware
>>> load_hardware().focalplane_radius_deg
INFO: Loaded focalplane for time stamp 2023-03-22 20:35:13.114490+00:00
INFO: Focalplane has 801 fibers that are stuck or broken
1.65

with this PR:

>>> import desimodel.io
>>> desimodel.io.load_platescale()['theta'].max() 
1.651501393

for instance, I could imagine that this change could break the reproducibility of the fiberassign intermediate products (the TILEID-{targ,sky,gfa,scnd,too}.fits files, where the files generated with this PR would have more targets (larger radius); I guess the assignment will be the same, as those extra targets won t get assigned, but I think we d want to keep an exact reproducibility of everything if possible.

so maybe propagate in the code some argument that would default to the current behavior; don t know if that s trivial or not...

@forero forero merged commit 3593804 into main Mar 22, 2023
14 checks passed
@araichoor
Copy link
Contributor

sorry Jaime for not having been responsive here.
addressing here my own question: I think it s good in terms of reproducibility for the main survey; good!

details:

I ve run fba_rerun --infiberassign $DESI_TARGET/fiberassign/tiles/trunk/011/fiberassign-011946.fits.gz --outdir $OUTDIR, with $OUTDIR:

  • main (before this PR): /global/cfs/cdirs/desi/users/raichoor/tmpdir/fiberassign_platescale_fix/main
  • branch (ie this PR): /global/cfs/cdirs/desi/users/raichoor/tmpdir/fiberassign_platescale_fix/branch

from a quick look, output files are identical, perfect!

I think the reason is that the targets (sky, gfa, too) are queried in fiberassign.fba_launch_io.py with desitarget.io. read_targets_in_tiles(); and that desitarget function calls desimodel.footprint.is_point_in_desi(), which calls desimodel.geometry.get_tile_radius_deg(); and that function proceeds a bit differently:
https://github.com/desihub/desimodel/blob/d18fc5a7ff0c7f2c9cafee890c8c0858ad524ed2/py/desimodel/focalplane/geometry.py#L34-L43

def get_tile_radius_deg():
    '''Returns maximum radius in degrees covered by the outermost positioner.
    '''
    global _tile_radius_deg
    if _tile_radius_deg is None:
        rmax = get_tile_radius_mm()
        platescale = load_platescale()
        fn = interp1d(platescale['radius'], platescale['theta'], kind='quadratic')
        _tile_radius_deg = float(fn(rmax))
    return _tile_radius_deg

and it returns 1.6280324520485583; which is a smaller radius than both radii mentioned in my previous message (1.65 and 1.651501393).
so the selected targets in the intermediate fiberassign files aren't impacted by this, I think.

@araichoor araichoor deleted the platescale_fix branch June 8, 2023 18:13
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

3 participants