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

Add DCR as a surface operation #54

Closed jchiang87 closed 6 years ago

jchiang87 commented 6 years ago

@danielsf I was poking around in the lsst code trying to find an hour angle calculation, but couldn't find anything that I could easily call from python. There's one in lsst.afw.VisitInfo, but using that class drags in a lot of stuff we don't need for this purpose (plus it wasn't clear how to use it), so I wrote one from scratch. If there's a better way to do this using existing lsst code, we should do that.

@rmjarvis Could you have a look at this to make sure I'm using the DCR correctly?

danielsf commented 6 years ago

There actually isn't a method in lsst_sims to calculate hour angle. The closest we come is this method to calculate local mean sidereal time

https://github.com/lsst/sims_utils/blob/master/python/lsst/sims/utils/CoordinateTransformations.py#L27

I don't feel strongly that this is a case where there should be only one way to do something. What you have done is fine.

danielsf commented 6 years ago

The only unit test failure I saw was

        self.assertAlmostEqual(321.62974517903774,
>                              gs_interpreter.getHourAngle(mjd, ra))
E       AssertionError: 321.62974517903774 != 321.6297535598362 within 7 places

This is less than a second of slop, so I would be okay with just relaxing the tolerance of assertAlmostEqual.

cwwalter commented 6 years ago

@rmjarvis We would like to get this PR merged this so we have DCR working. But, you made some technical suggestions on hour angle calculations and the GalSim types here that we don't think we are able to address right away due to our familiarity with these issues.

Would you be willing to actually try to make your suggested changes so we can get DCR into the code base? That would help us out right now.

rmjarvis commented 6 years ago

you made some technical suggestions on hour angle calculations and the GalSim types here that we don't think we are able to address right away due to our familiarity with these issues.

I thought I was pretty explicit about what the changes ought to be. What is the confusion?

The real problem seems to be that there is no unit test that actually runs the GalSimSiliconInterpeter.drawObject() method. If you do try to run it, I'm pretty sure you will get TypeErrors, which will be fixed by changing all those /galsim.degrees in that function to *galsim.degrees.

The other suggestions I made for the getHourAngle() method are really just style improvements, so if you don't care, you don't have to take them. I think the existing code is accurate. Just (IMO) clunky.

cwwalter commented 6 years ago

OK let me try ...

cwwalter commented 6 years ago

Merging in DCR for use with imSim. Note: currently this only is enabled if you are using the silicon model.