xpsi-group / xpsi

X-PSI: X-ray Pulse Simulation and Inference
Other
34 stars 20 forks source link

Remove deprecated is_secondary and set is_antiphased to False in all examples #323

Open sguillot opened 1 year ago

sguillot commented 1 year ago

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: capture

Any thought on this ?

sguillot commented 1 year ago

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

DevarshiChoudhury commented 1 year ago

@sguillot I suppose we can close this issue then?

sguillot commented 1 year ago

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

DevarshiChoudhury commented 1 year ago

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 commented 6 months ago

@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 commented 6 months ago

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 commented 6 months ago

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.

thjsal commented 1 week ago

I removed the issues that were essentially same as this one. Just need to remember also change the CustomPrior and remove -0.5 also from there, when changing antiphase to False.

I think for reproducing the old results, it could still be useful to keep the is_antiphased flag existing, but changing the default to False sounds good to me.

I also updated the Issue title to describe what should be done.