search

desihub / specsim

Quick simulations of spectrograph response
2 stars 9 forks source link

Re-implement Camera.get_output_resolution_matrix #50

Closed dkirkby closed 8 years ago

dkirkby commented 8 years ago

Camera.get_output_resolution_matrix() now returns a DIA-format sparse matrix, which uses significantly less memory and runs 4-5x faster.

A new unit test compares the previous dense downsampling algorithm with the new sparse algorithm to ensure that the values are np.allclose.

This is ready for review and merge and fixes #43.

Since the desispec.resolution.Resolution constructor accepts either a dense or DIA-sparse matrix, no API changes should be required for these desisim lines in quickbrick.py:

R = Resolution(camera.get_output_resolution_matrix())

and quickgen.py:

resolution_matrix = Resolution(
   qsim.instrument.cameras[i].get_output_resolution_matrix())

However, the internals of the Resolution constructor might need some work if the list of diagonals provided does not match what the brick datamodel expects.

sbailey commented 8 years ago

This looks good in that it is faster, much less memory hungry, and returns equivalent results. Thanks for the update, let's merge this now.

Heads up: as we exercise quickgen/quickbrick more we may have more requests about this. In particular I expect that we have a desispec constraint that the number of resolution matrix diagonals is odd (the diagonal + an equal number of upper+lower off diagonals), while specsim can currently return an even number of diagonals.