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 |
smericks
left a comment
smericks
left a comment
There was a problem hiding this comment.
I made some suggestions for documentation details. I also suggested to give a more informative key error message for 'source_amp' vs 'image_amp'. Overall, I do agree that image_amp makes more sense than point_amp.
| 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. :) |
4 participants
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.