Missing argument in `ImageData.reverse_se`

[suvayu]

Description

Mypy tells me:

sourcefinder/image.py:468: error: Missing positional argument "anl" in call to "extract"  [call-arg]

I don't know what should be the correct value of anl here, so I couldn't fix it.

Remarks

This method is not used anywhere in PySE. I'm guessing this was implemented because it might be useful elsewhere in TraP? If so, there should be explicit tests for the ImageData class testing all potential downstream API calls. There are no such tests.

[HannoSpreeuw]

Nice catch, there is a typo. It should be

 def reverse_se(self, det, anl):
 ...
    results = self.extract(det=det, anl=det)

ImageData.extract is a fundamental method of PySE and does appear in the unit tests, e.g. here. Perhaps there is no call to this method outside the tests because other methods do not depend on it. That is because it is a top level method. But perhaps you mean that ImageData.reverse_se is not used nor tested anywhere in PySE? Let me check that.

[suvayu]

Okk, I'll fix the typo.

Regarding tests, I meant both. The example you cite doesn't count, because it's not actually testing the API, it's part of the setup for another test. If we were to refactor ImageData.extract, that indirect use wouldn't help. We would see unrelated failures, which we would have to debug to realise it's an indirect effect of the refactor.

[jdswinbank]

Hey @HannoSpreeuw — I'm not sure that anl=det is right.

This seems to date back to April 2014, when somebody (cough) removed a default None as the value of anl in ImageData.extract (see here). That implies that the call in reverse_se should be:

    ...
    results = self.extract(det=det, anl=None)
    ...

(Untested, though).

That said, I'm pretty sure that reverse_se itself dates back into the mists of time and has probably never been used in anger. I'd guess it needs a bunch of thinking before putting it into practice.

[suvayu]

Thanks John, I'll hold off on the fix.

[HannoSpreeuw]

Indeed, reverse_se is not tested. I could write a unit test or we could remove it.

What is the use case? It could be that the reverse_se method is to applied not on Stokes I, but e.g. on Stokes V images, with a circularly polarized source. The peak spectral brightness at the position of the source is negative and high enough to be identified as a cosmic source. You can then extract this source using reverse_se.

But there is the comment:

        Obviously, there should be no sources in the negative image, so this
        tells you about the false positive rate.

So perhaps reverse_se should be applied on a Stokes I image - which positive pixel values from its "negation" should always be noise related - to determine what an appropriate threshold for source detection should be.

[jdswinbank]

Hi Hanno — it's been a long time, but I think your second use case (determining noise based on a negation of the Stokes I image) sounds like the original idea.

[HannoSpreeuw]

Allright. We will keep it and I will fix it and include a unit test, that should not be so hard.

[HannoSpreeuw]

anl=None will not work because of line 419 if anl > det, it yields:

TypeError: '>' not supported between instances of 'NoneType' and 'float'

but I'll think of something else.