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

forero commented 1 year ago

This PR fixes bug #452

forero commented 1 year ago

These changes only affect the function targets_in_tiles()

araichoor commented 1 year ago

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...

araichoor commented 1 year ago

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.

desihub / fiberassign

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