LSSTDESC / sims_GCRCatSimInterface

LSST sims interface to gcr-catalogs
BSD 3-Clause "New" or "Revised" License
1 stars 5 forks source link

Remove unneeded ICRS code and unify instance catalog outputs for imsim and phosim #47

Closed jchiang87 closed 6 years ago

jchiang87 commented 6 years ago

Now that imsim uses the same object coordinates as phosim and can now read gzipped files with includeobj entries, these changes remove the special ICRS conversions and creation of the special imsim_cat_*.txt catalogs. The only remaining differences are the FWHMeff, FWHMgeom, and rawSeeing commands that imsim needs. If we could remove the need for those, the catalogs could be identical.

cwwalter commented 6 years ago

I think in the future we might want the ability to make catalogs with ICRS since if users are making specialized catalogs themselves (as opposed to going through our CatSim machinery) figuring out all of the positions is burdensome.

Do you think it might make sense to leave those transforms in? Or should we just recreate/refind this code in the future if necessary. (I am partly putting this comment in to allow me to search for this PR in the future after it is closed if needed).

jchiang87 commented 6 years ago

I think we should put a switch in imsim to allow it to handle ICRS coordinates in that case. The way we are handling things here involves a separate distinct class for each InstanceCatalog class in the phosim case. When there were just a couple of those, that was not a problem, but the various additions from the sprinkled object types has made it something like a ~10 classes that need alternate implementations, with more on the way. There are probably other ways of handling the parallel sets of classes, but your use case indicates a switch in imsim is the place to handle catalogs with ICRS coordinates.

danielsf commented 6 years ago

I believe I have incorporated all of the changes from this PR into #52 (rebasing one onto the other PR was going to be annoying). Jim should verify that, though.