DM-34770: Use PSF average position explicitly in PSF methods #51
Conversation
| @@ -123,7 +123,7 @@ def _computePsfImage(self, position=None): | |||
| for bidx, single in enumerate(self.singles): | |||
| try: | |||
| if position is None: | |||
| psf = single.getPsf().computeImage() | |||
| psf = single.getPsf().computeImage(single.getPsf().getAveragePosition()) | |||
There was a problem hiding this comment.
I am a bit confused as to why computeImage is called when no position was specified and computeKernelImage is called when it is specified. This might have different behavior depending on whether or not the average position is passed in explicitly.
There was a problem hiding this comment.
Honestly, it should probably throw an exception, since using the average position can cause unexpected problems.
There was a problem hiding this comment.
Thinking back on this as to why that behavior is coded in the first place, computeKernelImage definitely didn't work properly if no position was given. At the time, computeKernel didn't have the same bug, so that's why we still allowed it. But I do think that raising an Exception if no position is specified is the correct behavior now.
There was a problem hiding this comment.
In that case, I can close this PR without merging and perhaps @fred3m can create another ticket to raise an exception?
There was a problem hiding this comment.
I think this branch is actually unused, which is good. So I think that DM-34777 is trivial.
There was a problem hiding this comment.
Sorry @arunkannawadi , it looks like our comments crossed and I didn't see yours before I submitted. So looking at that ticket I understand why this is being updated now. However, I stand by my original assertion that this function should never take position=None as an argument. Currently if one tries to use position=None in the stack this will display a future warning (I just tried with the latest weekly). I would prefer that to not notifying the user that their use of position=None is a bad idea. So if you don't want to raise an exception in this ticket please revert this change, as I prefer the current behavior that will at least notify people that they are doing something bad (or wait for DM-37777).
There was a problem hiding this comment.
Okay, I think the thing to do is not make this change, and add a TODO DM-37777 which hopefully is done soon.
There was a problem hiding this comment.
I thought the point of this ticket was to make it so that the future warning was handled? If position=None is not allowed then raise, don't wait for the warning.
There was a problem hiding this comment.
I did just finish DM-34777 and if Arun prefers, he can just merge it into this ticket branch instead of main. As @erykoff said, the change was trivial.
arunkannawadi
force-pushed
the
tickets/DM-34770
branch
from
7075760
to
457f27b
Compare
| @@ -91,7 +91,7 @@ def _checkBlendConvergence(blend, f_rel): | |||
| def _getPsfFwhm(psf): | |||
| """Calculate the FWHM of the `psf` | |||
| """ | |||
| return psf.computeShape().getDeterminantRadius() * 2.35 | |||
| return psf.computeShape(psf.getAveragePosition()).getDeterminantRadius() * 2.35 | |||
There was a problem hiding this comment.
This method doesn't seem to be used anywhere. If it's just not showing up in my Github search, then this should take position as an argument and compute the PSF shape at that position.
There was a problem hiding this comment.
I think that whatever called it was removed some time ago, so you should be able to just remove _getPsfFwhm.
There was a problem hiding this comment.
Ah great, note that the use here (and above!) were found by searching through the code, and did not show up in the logs in RC2 processing. So I think this really is unused.
There was a problem hiding this comment.
Upon further review/consideration, I think this should be removed on DM-34777, and this PR should be closed as a noop.
| @@ -91,7 +91,7 @@ def _checkBlendConvergence(blend, f_rel): | |||
| def _getPsfFwhm(psf): | |||
| """Calculate the FWHM of the `psf` | |||
| """ | |||
| return psf.computeShape().getDeterminantRadius() * 2.35 | |||
| return psf.computeShape(psf.getAveragePosition()).getDeterminantRadius() * 2.35 | |||
There was a problem hiding this comment.
Upon further review/consideration, I think this should be removed on DM-34777, and this PR should be closed as a noop.
|
Agreed on closing this without merging. Since the spirit of DM-34770 is to provide the appropriate position argument to PSF methods, removal of |
|
I did include the change in DM-34777 so it is OK to close this PR. FTR it took less time to implement this PR (~30 seconds to code and 1 minute to test) than we spent arguing about where/when the change should be made. In the future, trivial changes like that shouldn't require a new ticket, especially when they are within the scope of the original ticket. |
|
Agreed, and apologies. I didn't want to remove or incorrectly suppress warnings without knowing the intentions of the original author. |
No description provided.