search

lsst-sims / legacy_sims_GalSimInterface

code to seamlessly generate simulated images using GalSim based off of simulated catalogs generated by CatSim
5 stars 7 forks source link

implement GalSimInterpreter.drawFitsImage enable rendering of FITS images #57

Closed jchiang87 closed 6 years ago

jchiang87 commented 6 years ago

These changes work with the issue/83/static_lens_images branch of imSim. @danielsf It looks like there may need to be additional work in galSimCatalogs.py to support the changes to GalSimCelestialObject, but it wasn't clear to me exactly what's needed.

danielsf commented 6 years ago

I doubt that anyone uses the galSimCatalogs API. Now that this package is primarily the middleware for ImSim, I wonder if it is worth jettisoning galSimCatalogs.py for ease of maintenance....?

jchiang87 commented 6 years ago

I didn't realize that galSimCatalogs.py wasn't used any more. In that case, I'm ok with removing it.

danielsf commented 6 years ago

I did a grep for all of the sims.GalSimInteface imports in ImSim. I did not see anything that needed galSimCatalogs.py. If I remove it, is there either

a) a unit test in ImSim that will prove to me that I didn't break anything or b) a quick test you can run to prove to me that I didn't break anything?

(I'm about to board a plane out of Tucson; I will be in Boston on vacation next week, so, not totally incommunicado, but not in a terribly useful state, either)

jchiang87 commented 6 years ago

I removed galSimCatalogs.py and galSimPhoSimCatalogs.py and the imSim unit tests all ran fine, and I ran imsim.py with an instance catalog that has all of the different object types and that also ran fine. I think it would be ok to remove those files as part of a separate PR.

danielsf commented 6 years ago

When I ran this branch through Jenkins, I only got failures in test_getStampBounds in testGalSimInterface.py.

https://ci.lsst.codes/blue/organizations/jenkins/stack-os-matrix/detail/stack-os-matrix/28427/tests

jchiang87 commented 6 years ago

Thanks, Scott. I've updated the test code to account for the change to the GalSimCelestialObject constructor.

jchiang87 commented 6 years ago

Sorry, I should have checked for all of the places in the code where that constructor is called. I'll fix the galSimCatalogs.py case.

We could make all of the GalSimCelestialObject.__init__ arguments into args, but given the all the arguments that have been added, I'm wondering if it might be better to make a separate subclass for each galsim type. This might make it easier to add new types of objects to render.

jchiang87 commented 6 years ago

I'll merge this at 3 pm PT today unless I hear otherwise.