Point source refactoring #338
Conversation
…d in the lensed image plane instead of the source plane. This should improve the distinction between lensed and unlensed amplitudes.
Pull Request Test Coverage Report for Build 2857037590
💛 - Coveralls |
| @@ -8,7 +8,7 @@ class LensedPositions(PSBase): | |||
| """ | |||
| class of a a lensed point source parameterized as the (multiple) observed image positions | |||
| Name within the PointSource module: 'LENSED_POSITION' | |||
| parameters: ra_image, dec_image, point_amp | |||
| parameters: ra_image, dec_image, source_amp/source_amp | |||
There was a problem hiding this comment.
Change to image_amp/source_amp
There was a problem hiding this comment.
It might be helpful to explicitly write "source_amp is the amplitude of the point source in the source plane. image_amp are the amplitudes of the point source images in the image plane. When fixed_magnification=True, use source_amp. Otherwise, use image_amp" (in source_position as well)
| @@ -11,7 +11,7 @@ class of a single point source defined in the original source coordinate positio | |||
|
|
|||
| Name within the PointSource module: 'SOURCE_POSITION' | |||
| parameters: ra_source, dec_source, source_amp, mag_pert (optional) | |||
There was a problem hiding this comment.
change to " parameters: ... image_amp/source_amp" to match lensed_position.py
| @@ -76,7 +76,7 @@ def image_amplitude(self, kwargs_ps, kwargs_lens=None, x_pos=None, y_pos=None, m | |||
| mag = self._lens_model.magnification(ra_image, dec_image, kwargs_lens) | |||
| point_amp = kwargs_ps['source_amp'] * np.abs(mag) | |||
There was a problem hiding this comment.
Is there some way to throw an informative error before trying to access kwargs_ps['source_amp']? i.e. check for both self.fixed_magnification = True and the existence of the 'source_amp' key before trying to access it? And also check the opposite for self.fixed_magnification = False and 'image_amp'. This might be difficult because you would have to put this in every function that uses kwargs_ps...
|
@ajshajib : ah, not yet. It's on my todo list and then I re-post it to Sydney. This branch currently also has conflicts that need to be resolved. Since this current proposed change is not backwards compatible, I was thinking of merging this instead into an other beta_release branch where we might accumulate multiple features/conventions that are not backward compatible but generally a good idea to change |
Ok, got it. No worries or rush from my side. :) |
The current convention for the point source amplitudes are 'point_amp' for amplitudes in the image plane and 'source_amp' for amplitudes in the source plane. This lead to confusions about in what plane the amplitudes are defined, since this convention could be equally applied to 'SOURCE_POSITION' and 'LENSED_POSITION' point source models.
This PR suggests to rename 'point_amp' -> 'image_amp'. This would require a non-backwards compatible change but might provide more clarity moving forward. Suggestions are welcomed.