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

Add new DESI-0347 throughput files #29

Merged
merged 5 commits into from Oct 14, 2016
Merged

Add new DESI-0347 throughput files #29

merged 5 commits into from Oct 14, 2016

Conversation

dkirkby
Copy link
Member

@dkirkby dkirkby commented Oct 5, 2016

The new files are needed to support more realistic simulations and future pipeline development.

Since data files are maintained in svn, this github PR only shows the associated code and docs, which are contained within a jupyter notebook that you can browse here (this link will break after the merge).

I will also be adding the following new data files under $DESIMODEL/data via svn updates:

  • inputs/throughput/raytracing.txt: original file from Mike Sholl used to calculate the new files.
  • throughput/DESI-0347_blur.ecsv: spot size as a function of wavelength and field angle.
  • throughput/DESI-0347_offset.ecsv: nominal radial centroid offset as a function of wavelength and field angle.
  • throughput/DESI-0347_random_offset_<n>.ecsv: random (x,y) centroid offsets generated with random seeds n=1,2,3.

Let me know if you have any questions or concerns before I add these 6 files to the desimodel svn repo.

@dkirkby dkirkby assigned dkirkby and sbailey and unassigned dkirkby Oct 5, 2016
@sbailey
Copy link
Contributor

sbailey commented Oct 5, 2016

throughput/DESI-0347_random_offset_<n>.ecsv : isn't this a per-exposure randomness due to fiber positioners not ending up exactly where we had hoped? i.e. they should be generated for each exposure as part of the simulations, not encoded as a parameter file in desimodel. i.e. it seems like the generate_offsets code in the jupyter notebook should be part of a desimodel.focalplane library accessible to simulation code.

OK on the other files, which describe the static optics model that applies to all exposures (IIUC).

@dkirkby
Copy link
Member Author

dkirkby commented Oct 5, 2016

@sbailey wrote:

throughput/DESI-0347_random_offset_.ecsv : isn't this a per-exposure randomness due to fiber
positioners not ending up exactly where we had hoped?

Its more complicated than that, but certainly some fraction of these "random" offsets will vary between exposures. I agree that the function which generates these random offsets should callable from other code, and optionally called by quickgen and friends. There are several reasons why I wanted to also include some pre-generated offsets:

  • to define a nominal specsim simulation of DESI which does not require running any DESI-specific code.
  • to allow straightforward comparisons where something else changes but these offsets are kept fixed.
  • to allow comparisons of the impact of varying only these offsets without introducing additional random noise.

In principle, these files are redundant if we have reproducible random seeds (which I believe we do for numpy.random) so I could be convinced to pare this down to a single random realization (which is still need to support the first item).

@dkirkby
Copy link
Member Author

dkirkby commented Oct 5, 2016

@sbailey wrote:

OK on the other files, which describe the static optics model that applies to all exposures (IIUC).

throughput/DESI-0347_blur.ecsv combines the raytracing blur with the achromatic random blurs in cells B14:B26 of the Throughput sheet in DESI-0347. Since these achromatic blurs are dominated by static manufacturing and alignment errors, I think this is the right approach, at least for now.

@sbailey
Copy link
Contributor

sbailey commented Oct 5, 2016

OK to include some static realizations of the offsets for the reasons you describe, but please move the code for generate_offsets into a desimodel.focalplane module.

@dkirkby
Copy link
Member Author

dkirkby commented Oct 5, 2016

please move the code for generate_offsets into a desimodel.focalplane module.

I guess you mean add this function to the existing desimodel/py/focalplane.py ? (but not as a method of the existing FocalPlane class).

@sbailey
Copy link
Contributor

sbailey commented Oct 5, 2016

The pre-existing focalplane.py / FocalPlane class was abandoned before it was fully vetted and used. Refactor and update as needed, but I think that is the conceptual place for this code to go. As an alternative, we might reserve desimodel for strictly describing the hardware, while using desisim as the place for throwing random numbers based upon that hardware description...

@dkirkby
Copy link
Member Author

dkirkby commented Oct 6, 2016

I just completed the svn side of this as follows:

desimodel.svn% svn commit -m 'Add new throughput files described in https://github.com/desihub/desimodel/pull/29'
Adding         data/inputs/throughput/raytracing.txt
Adding         data/throughput/DESI-0347_blur.ecsv
Adding         data/throughput/DESI-0347_offset.ecsv
Adding         data/throughput/DESI-0347_random_offset_1.fits
Adding         data/throughput/DESI-0347_random_offset_2.fits
Adding         data/throughput/DESI-0347_random_offset_3.fits
Transmitting file data ......done
Committing transaction...
Committed revision 6804.

I believe the following commands are the simplest way to add these files to an existing desimodel git clone (can you confirm @weaverba137?)

cd $DESIMODEL
rm -rf data
install_desimodel_data

@dkirkby
Copy link
Member Author

dkirkby commented Oct 6, 2016

This PR is now ready for final review and merge.

@weaverba137
Copy link
Member

As long as install_desimodel_data executable is the same as the one in $DESIMODEL, it should work.

@dkirkby
Copy link
Member Author

dkirkby commented Oct 6, 2016

What might fail if you pick up the script from a different install of desimodel? Can we ensure that the correct script is called using:

python -c "from desimodel.install import main; main()"

I guess this could also fail if a different checkout of desimodel is installed? Perhaps the safest approach is to install first, but this seems heavy handed:

cd $DESIMODEL
rm -rf data
python setup.py install (or develop)
install_desimodel_data

Everyone running simulations will need to perform these steps to update their desimodel, so these instructions will need to be advertised and used.

@weaverba137
Copy link
Member

Unless I'm missing something, I think the instructions in the README.rst file cover the various install scenarios.

@dkirkby
Copy link
Member Author

dkirkby commented Oct 6, 2016

Those instructions don't explicitly cover the scenario most relevant here, where you have already installed data/ but now need to re-synch with changes to the svn desimodel.

@weaverba137
Copy link
Member

OK, but that only applies if you're working out of a git clone/svn checkout of desimodel. For tagged versions, this would never arise.

Sounds like the correct answer is for the data directory to be an svn checkout, not an export. So you just svn up instead of removing the whole thing.

@dkirkby
Copy link
Member Author

dkirkby commented Oct 9, 2016

Last call for feedback on this PR.

@sbailey How do we close the loop with the engineering team?

@sbailey
Copy link
Contributor

sbailey commented Oct 11, 2016

What's the format of the DESI-0347_random_offset_<n>.fits files on the svn side? I was expecting arrays of 5000 offsets for a nominal location per positioner. They are 256x266 2D arrays, but I don't see header keywords indicating how to interpret that.

For the engineering team, we want them to take ownership of these files and put them in DESI-0347?

  • data/inputs/throughput/raytracing.txt
  • data/throughput/DESI-0347_offset.ecsv
  • data/throughput/DESI-0347_blur.ecsv

i.e. when there are further updates to those data, they should provide updates back to us via that format? I can talk to them about that to confirm it, but we could proceed with putting them in desimodel in the meantime.

For the record, I remain nervous about column names like "r=0.15 deg", though I haven't found a case where that actually breaks (though I've only tested within python for astropy Table, numpy structured array, and pandas). We may need to revisit this if it does actually break something.

@dkirkby
Copy link
Member Author

dkirkby commented Oct 12, 2016

A random offset file provides a uniform grid of centroid offsets over the focal plane, as in this example taken from the supporting notebook:
random_offset_2

The only FITS headers right now are:

    header['COMMENT'] = 'Random focal plane centroid offsets in microns.'
    header['SEED'] = seed
    header['UNIT'] = 'um'

but I can add anything else you think would be useful.

I would only add data/inputs/throughput/raytracing.txt to DESI-0347 to make it clear that the engineering team is responsible for updating the ZEMAX model and generating new ray tracing results, and we are responsible for propagating changes to the simulations and pipeline via the other files (by re-running the code in the notebook).

@sbailey
Copy link
Contributor

sbailey commented Oct 13, 2016

Thanks. I was mis-interpreting which random offsets were being captured in the random offsets files.

Agreed on engineering owning raytracing.txt and we own the format conversion and the other files.

I'd suggest using the FITS header keywords CRPIXn, CRVALn, CTYPEn, and CDELTn to define what the range of 256x256 corresponds to in degrees on the focal plane instead of being effectively hardcoded in the fov=1.6 * u.deg parameter of plot_random_offsets. Beware: FITS defines CRPIXn as 1-indexed, not 0-indexed. Then the random offset files would be self contained.

@dkirkby
Copy link
Member Author

dkirkby commented Oct 14, 2016

@sbailey Can you recommend a good reference for defining a basic linear WCS? All the references I am finding make it sound very complicated.

The file currently has two images (dx, dy) in HDUs 1, 2. Should the WCS header appear (1) only in HDU0 (2) only in HDUs 1 and 2 or (3) all three HDUs?

@dkirkby
Copy link
Member Author

dkirkby commented Oct 14, 2016

This is what I have so far, but I am just guessing at a lot of this:

    import astropy.wcs as wcs

    scale = fov / npix
    w = wcs.WCS(naxis=2)
    w.wcs.ctype = ['X', 'Y']
    w.wcs.crpix = [npix / 2., npix / 2.]
    w.wcs.cdelt = [scale, scale]
    w.wcs.crval = [0., 0.]
    header = w.to_header()
    header['BUNIT'] = 'um'
    header['COMMENT'] = '+x component of focal plane offset.'

How do I specify the units of CDELTn (degrees)? Are there predefined values for CTYPEn that I should be using to indicate "field angle" ?

@sbailey
Copy link
Contributor

sbailey commented Oct 14, 2016

Using the CRPIXn, CRVALn, CTYPEn, and CDELTn header keywords is much simpler than full WCS when just specifying a 2D cartesian grid without any distortions, tangent plane projections, etc. See

http://heasarc.gsfc.nasa.gov/docs/fcg/standard_dict.html

In this case I think you would want:

  • CRPIX1 = CRPIX2 = 1 (reference pixel; 1-indexed)
  • CRVAL1 = CRVAL2 = -0.8 (coordinate value at reference pixel)
  • CDELT1 = CDELT2 = 1.6 / (256-1) (stepsize from one pixel to the next; check that)
    • Note: CDELTn, not CRDELTn
  • CTYPE1 = 'x', CTYPE2 = 'y'
  • BUNIT = whatever the units are for the values in the 256x256 array
  • unfortunately I don't see a way to specify the units of the axes themselves

@sbailey
Copy link
Contributor

sbailey commented Oct 14, 2016

Typing on top of each other. I think your solution is approximately right too, using a reference pixel in the middle rather than the corner. However, CRPIXn is 1 indexed (alas) and refers to the middle of the pixel. Since you have even-sized arrays your origin=(0,0) is on the boundary between pixels, so I think you want CRPIXn = npix/2. + 0.5 .

@dkirkby
Copy link
Member Author

dkirkby commented Oct 14, 2016

I updated the notebook to write and read the new offset FITS files. The reader uses wcslib to interpret the header keywords and build the plot axes, so I am reasonably confident that things are kosher now. If this looks good, I will go ahead and update the FITS files in the SVN desimodel.

@sbailey
Copy link
Contributor

sbailey commented Oct 14, 2016

Looks good. Thanks. Go ahead and update the files in svn and merge this when you are ready.

@dkirkby
Copy link
Member Author

dkirkby commented Oct 14, 2016

I just updated the 3 files in svn and will now merge this PR.

@dkirkby dkirkby merged commit 2806d06 into master Oct 14, 2016
@dkirkby dkirkby deleted the ThroughputUpdates branch October 14, 2016 23:39
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