Possible bug in HotRegion.py

[sguillot]

The @is_secondary.setter method of the HotRegion class should check that is_secondary is a boolean, and set it to self. But the fonction is coded with is_antiphased. Is this normal ?

@is_secondary.setter
def is_secondary(self, is_secondary):
    if not isinstance(is_antiphased, bool):
        raise TypeError('Use a boolean to specify whether or not the '
                        'hot region should be shifted by half a cycle.')
    else:
        self._is_antiphased = is_antiphased

Also, the worst part is that is_antiphased is undefined in this method. see screen capture:

Any thought on this ?

[sguillot]

Ah, I see in the doc string that it is deprecated apparently...which it why it doesn't cause crashed.

[DevarshiChoudhury]

@sguillot I suppose we can close this issue then?

[sguillot]

Well I'm not sure.
I think it should be fixed, or removed from the HotRegion if it is deprecated, no ?

[DevarshiChoudhury]

I think it was originally retained so that people running any old scripts can continue using it. Although I suppose, by now there have been so many changes since that version that it is probably better to just remove it, and have people run those scripts using the relevant X-PSI versions

[DevarshiChoudhury]

@sguillot, @thjsal if you agree with the above proposition of removing the deprecated argument, I can change all the is_secondary to is_antiphased

[sguillot]

Yes, I think so. (and all the occurence in tutorial notebooks). I had meant to do it but never got the chance to actually do so.

At the same time, it would be good to solve #296 (and #291) if you can

[thjsal]

Removing the deprecated is_secondary sounds fine to me. And if I got it right, @sguillot wants also is_antiphased to be removed, or at least set to False in all our examples and analyses. Both options are fine to me. But good to then keep in mind that one of our sampled parameters is then defined differently than before, and that should be clearly noted in the future
papers.