search

GalSim-developers / GalSim

The modular galaxy image simulation toolkit. Documentation:
http://galsim-developers.github.io/GalSim/
Other
227 stars 107 forks source link

Switch the (x,y) order convention in LookupTable2D #1103

Closed rmjarvis closed 4 years ago

rmjarvis commented 4 years ago

@jmeyers314 Here is what it would look like to switch the convention of LookupTable2D. I think this is a net positive, since most of the changes to unit tests and such were to get rid of transposes that were used in various places to account for the reverse convention being used here.

However, there are two down-sides of which I'm aware:

  1. This is an API change. I called it out in the CHANGELOG, but this isn't one that would be easy to go through a deprecation stage. I think it's minor enough that it doesn't require a major version number, but we could go to 3.0 if you think that would be more appropriate. I'm fine either way.
  2. The internal calculations in T2DGSInterpolant::interp are now a bit slower. Reversing the storage order means that the sums now cut across the rows rather than go along them. If we decide we want to do this, I may try out a couple optimizations to see if I can recover any of that time. But also, I don't think this calculation is very often a tall pole, so it might not really matter.

This PR is set to merge into your branch, so you can evaluate this on those terms. It includes removing the suspect .T that I called out.

jmeyers314 commented 4 years ago

Thanks, this looks good to me.

As for versioning, I agree the current change probably doesn't merit a major increment. I do want to go through and decide if our wavefronts/OpticalPSF stuff ought to better match Zemax though, and if that leads to changes it may be a good time to consider a major version bump. I know Aaron has pointed out for years that there are differences here, so we probably need to at least better document the various coordinate systems involved.

I also think you're right that T2DGSInterpolant::interp being a bit slower is nobody's tall pole. So I'm happy to merge this into the TableScreen branch if you like.

jmeyers314 commented 4 years ago

Oh, one more comment. With your changes, now T2DGSInterpolant::interp looks a lot more like XTable::interpolate. So might be worth trying to deduplicate.

rmjarvis commented 4 years ago

I have a long-term goal to refactor much of XTable and KTable. They used to be used for all the FFT stuff, but now they are just an implementation detail of SBInterpolatedImage, really, since I moved the primary FFT actions to the Image class. I think most of the XTable and KTable functionality could be similarly treated, but I ran out of motivation at the time I was working on that. So I'll keep it in mind to include this function in the refactoring to deduplicate some of this. It's not a particularly high priority I guess though.

rmjarvis commented 4 years ago

Oh, one more comment about the conventions in Zernike. I also think some of those might have the 1j in the wrong place, which might explain some of the convention mismatches you were seeing. In particular in _xy_contribution the 1j multiplies the j term, which I think is backwards. I didn't try switching it, but I'd guess it would break some existing unit tests, so you might try to think through whether that should be changed or not.

rmjarvis commented 4 years ago

Anyway, merging this onto your branch now...