ENH: add a conditional to adapt behaviour depending on astropy's version

[neutrinoceros]

likely a better approach than #59
closes #31
follow up to #45

This should not be merged before Astropy 5.0.0 is out and we can properly test against it.
Note that I'm adding packaging as a hard dependency to parse versions. This is the officially recommended way to do this after the equivalent functionality in setuptools was deprecated and targeted for removal in Python 3.12 (expected late 2023).

[codecov]

Codecov Report

Merging #60 (9188c56) into master (bb3ce8e) will increase coverage by 0.50%.
The diff coverage is 87.50%.

❗ Current head 9188c56 differs from pull request most recent head 282e12f. Consider uploading reports for the commit 282e12f to get more accurate results

@@            Coverage Diff             @@
##           master      #60      +/-   ##
==========================================
+ Coverage   35.01%   35.52%   +0.50%     
==========================================
  Files          28       28              
  Lines        8264     8267       +3     
==========================================
+ Hits         2894     2937      +43     
+ Misses       5370     5330      -40     

Flag	Coverage Δ
unittests	35.52% <87.50%> (+0.50%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
amical/mf_pipeline/bispect.py	76.08% <87.50%> (-0.05%)	⬇️
amical/externals/pymask/cp_tools.py	19.02% <0.00%> (+5.53%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bb3ce8e...282e12f. Read the comment docs.

[vandalt]

Indeed, this looks like a good way to support older Astropy versions and to keep commentary cards when possible. Do you think it would be worth adding a warning saying that commentary cards are removed from the header when using Astropy < 5.0 ?

[neutrinoceros]

Excellent idea, feel free to push to this branch if you want to add this warning !

[neutrinoceros]

@vandalt you're still welcome to push here if you're interested. It's also ok if you're not. Keep me tuned :)

[vandalt]

Hello @neutrinoceros, just a few questions:

• Should I edit the branch on your fork directly (if yes I think I'll need permission to push) or should I start a new PR based on this one ? Or maybe the best is just to use suggestions via Github ?
• Given that the file has warnings.filterwarnings("ignore") and that the whole code base seems to use cprint instead of actual warnings, I would add the warning with verbose (which I would set to true by default in this function) and cprint. Is that OK ?

[neutrinoceros]

Should I edit the branch on your fork directly (if yes I think I'll need permission to push) or should I start a new PR based on this one ? Or maybe the best is just to use suggestions via Github ?

I didn't realize you'd need explicit permission to edit my fork (which makes sense actually). I'd rather you go with a follow up PR then.

Given that the file has warnings.filterwarnings("ignore")

Damn it I forgot about that. This is a big weakness in AMICAL because it has very undesirable side effects: the state of the warnings module is shared globally. This means that importing AMICAL disables all warnings emitted, not just the ones emitted from inside AMICAL itself.
I really don't think we should embrace this and it ought to be fixed at some point, but that's a much more substantial change than the one we're discussing here, and indeed it is blocking for you here.

I'll open an issue from this comment.

[vandalt]

Sounds good. I'll make a follow-up PR this weekend (astropy 5.0 feature freeze is this Friday so it will be a good time to test) and use cprint for the warning, to stay consistent with the rest of AMICAL. As you say moving to warnings is a much bigger change, so I suggest we move forward with this change and change to a real warning if/when the whole code base does.

[neutrinoceros]

superseded by #73