Fix bug that arose when users would supply --fixed-posns in the cli

[tmillenaar]

We referred to the wrong attribute in the pyse script regarding fixed positions.

[tmillenaar]

I also added a fixed position to the test_pyse command, but that runs into the problem that fixed positions do not work together with residuals export. If this is intended, we can add an error here:

pyse/sourcefinder/utility/cli.py

Line 491 in b316313

elif conf.image.detection_image:

We would then also have to make a separate test if we wanted to test the fixed position in test_pyse, as to not test for both fixed positions and residual export at the same time.

If they are meant to work together, @HannoSpreeuw I would like to hear your thoughts because with the current setup I do not see a natural way to make the forced location sample work with the gaussian residuals.

[HannoSpreeuw]

I also added a fixed position to the test_pyse command, but that runs into the problem that fixed positions do not work together with residuals export. If this is intended, we can add an error here:

pyse/sourcefinder/utility/cli.py

Line 491 in b316313

elif conf.image.detection_image:

I am not sure I understand this comment about residuals export, since conf.image.detection_image is about an input image.

[HannoSpreeuw]

Closes #177

Not directly related to issue #177 nor to this MR, but the test_pyse_export unit test depends on PySE's printed output:

        if "Number of detected sources" in line:

which depends on the words chosen here:

        print(
            f"Number of detected sources ="
            f" {num_islands_above_detection_threshold}"
        )

i.e. if I would change those lines to:

        print(
            f"Number of found sources ="
            f" {num_islands_above_detection_threshold}"
        )

or simply remove the print statement, the unit test will always fail.
(It fails now for a different reason)

Is there a way to test the cli output without depending on (the words chosen in) a print statement?

[tmillenaar]

I also added a fixed position to the test_pyse command, but that runs into the problem that fixed positions do not work together with residuals export. If this is intended, we can add an error here:

pyse/sourcefinder/utility/cli.py

Line 491 in b316313

elif conf.image.detection_image:

I am not sure I understand this comment about residuals export, since conf.image.detection_image is about an input image.

It is not about that one line, but about the section that line is in.
The block looks like this

if fixed_coords:
        if conf.image.fdr:
            parser.error("--fdr not supported with fixed positions")
        elif conf.image.detection_image:
            parser.error(
                "--detection-image not supported with fixed positions"
            )
        mode = "fixed"  # mode 2 above

Here we have some error modes if fixed_coords is combined with either conf.image.dfr or image.detection_image.
I was wondering whether it makes sense to also add an error condition when fixed_coords is combined with conf.export.residuals.

[tmillenaar]

Is there a way to test the cli output without depending on (the words chosen in) a print statement?

Sure, that would would be possible. We could put this information in some export product for example. I don't think that fundamentally cnahges the problem. We would still have the test adhere to the semantics of the export product, now we match the manner in which we print to stdout. Indeed if you change the text in the print line the test would break. You would be notified by the cicd that the test breaks and I hope it would be clear that this happened because of the text parsing. If you wanted to remove the line entirely we would indeed have to come up with a way of including this information in the export. We could also decide to not check the number of sources at all of course. It just made sense to me to verify this number since it was accessible and made sense to me to check, but you can also decide to ignore the stdout and be more rigorous on the csv checks for example.

In general I am quite liberal in modifying or even removing tests if the landscape around the tests change. To me this is only natural, though it is good practice to at least write a new test to verify the new behavior if the old test no longer makes sense.

[tmillenaar]

After mentioning checking the exports instead of the stdout, I decided to remove the stdout parsing of test_pyse and now check the number of rows in the csv instead. You can now change the text of the stdout messages without breaking the test

[HannoSpreeuw]

In which cases could we have len(df) > 1?

[HannoSpreeuw]

And why does it pass now? Before this commit it didn't.

[tmillenaar]

We would get a different number of sources if you lower the detection and analysis thresholds or use a different image for testing.

It passes now because I split test_pyse into two tests. One test tries all export options as before and the other specifies a specific target location. Combining the target location with residuals export or island export is a problem. This is why I was suggesting an error message here: #181 (comment)

[HannoSpreeuw]

You can now change the text of the stdout messages without breaking the test

Nice! This makes the test more robust, appreciated.

[HannoSpreeuw]

If they are meant to work together, @HannoSpreeuw I would like to hear your thoughts because with the current setup I do not see a natural way to make the forced location sample work with the gaussian residuals.

I guess I am finally catching up with you on this point, your understanding is deeper than mine; it was not immediately clear to me why there would be no Gaussian residuals if fixed_coords==True.
The code that is run in that case is fit_fixed_positions, right?
That returns a tuple of successful fits.
And _pyse, the function that computes Gaussian residuals - and source parameters - is not run.

If they are meant to work together, @HannoSpreeuw I would like to hear your thoughts because with the current setup I do not see a natural way to make the forced location sample work with the gaussian residuals.

No, they're not meant to work together in the sense that they are a "natural combination of settings". With some coding effort, you could compute residuals with fixed coordinates, though. But that should be a separate issue and MR.

I was wondering whether it makes sense to also add an error condition when fixed_coords is combined with conf.export.residuals.

Please do so.