search

kammerje / spaceKLIP

Pipeline for reducing JWST high-contrast imaging data. Published in Kammerer et al. 2022 and Carter et al. 2022.
https://ui.adsabs.harvard.edu/abs/2022SPIE12180E..3NK/abstract
MIT License
16 stars 10 forks source link

HOTFIX: Remove deprecated parameter from phase_cross_correlation #164

Closed AarynnCarter closed 5 months ago

AarynnCarter commented 5 months ago

The return_error keyword has been removed from phase_cross_correlation: https://scikit-image.org/docs/stable/api/skimage.registration.html#skimage.registration.phase_cross_correlation

Removing from our code too to avoid crashes. Previously we had this set to True, but it seems that this is returned regardless now, so no major changes needed.

mperrin commented 5 months ago

What version of skimage did the parameter get removed in? Consider having this PR also update the installation requirements or dependencies, if relevant.

Specifically, if someone is running with an older version of skimage, which does have and need that parameter, they would not get the returned error, and the code would probably raise an exception of not having the expected number of outputs from that function.

AarynnCarter commented 5 months ago

It was removed as of 0.23 (the current stable release). Fortunately, the default for 0.22 was to have return_error=True, so we were really just repeating that anyways, and I don't think we'll bump into any issues with not having the correct number of expected outputs. https://scikit-image.org/docs/0.22.x/api/skimage.registration.html#skimage.registration.phase_cross_correlation

mperrin commented 5 months ago

To be specific, my question is, if someone tries to install spaceKLIP in an environment with version 0.21, or 0.20, or older, does any error message tell them not to do that?

Currently, requirements.txt doesn't even list any minimum versions of any packages, nor does pyproject.toml. In general that's not ideal, because we're not really telling users anywhere "hey, this won't work if you have versions older than X"

AarynnCarter commented 5 months ago

The default value of return_error in older versions of phase_cross_correlation is True. So, assuming someone has an older version installed, there would be no negative impact to removing us the manual setting that parameter in the function call. The code will still work.

I agree that we should think about defining minimum requirements, but that seems to be a broader effort (which I will make an issue for now).