Add show_clean_params call to select_clean_data

[vandalt]

This closes #77, enabling users to use a single dictionary to specify parameters for the whole cleaning step, and integrating the plotting of cleaning parameters to the actual cleaning procedure. I also removed the show_clean_params call in the SPHERE example to avoid duplicating plots.

I also added the SPHERE data directory and the Saveoifits directory to .gitignore, as I realized they were not there when I ran the examples. It's not directly link to the API change, but I wanted to avoid another PR just of this small change.

[codecov]

Codecov Report

Merging #78 (a96206a) into master (a836ddf) will decrease coverage by 23.34%.
The diff coverage is 0.00%.

❗ Current head a96206a differs from pull request most recent head 56cff84. Consider uploading reports for the commit 56cff84 to get more accurate results

@@             Coverage Diff             @@
##           master      #78       +/-   ##
===========================================
- Coverage   71.53%   48.19%   -23.35%     
===========================================
  Files          25       19        -6     
  Lines        3833     3648      -185     
===========================================
- Hits         2742     1758      -984     
- Misses       1091     1890      +799     

Flag	Coverage Δ
unittests	48.19% <0.00%> (-23.35%)	⬇️

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

Impacted Files	Coverage Δ
amical/data_processing.py	7.22% <0.00%> (-77.05%)	⬇️
amical/tools.py	30.67% <0.00%> (-49.33%)	⬇️
amical/mf_pipeline/ami_function.py	58.39% <0.00%> (-24.88%)	⬇️
amical/mf_pipeline/bispect.py	76.28% <0.00%> (-17.37%)	⬇️
amical/calibration.py	60.13% <0.00%> (-11.12%)	⬇️
amical/mf_pipeline/idl_function.py	85.71% <0.00%> (-9.53%)	⬇️
amical/oifits.py	72.16% <0.00%> (-8.92%)	⬇️
amical/tests/test_extraction.py	97.43% <0.00%> (-2.57%)	⬇️
amical/get_infos_obs.py	74.46% <0.00%> (-1.62%)	⬇️
... and 14 more

---

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 a024ac8...56cff84. Read the comment docs.

[neutrinoceros]

It's not your fault this function isn't yet tested, but maybe consider fixing that too while you're at it :)

[vandalt]

These are quite big functions with a lot of cleaning steps performed and they are all pretty new for me, so I'm not sure I would be able to write tests for it in the short term. However, it is definitely something I can look into in the near future, as I experiment more with the data cleaning procedure. Given that this change does not affect the calculations or the outputs of the function, would it be OK to defer adding tests for cleaning to a future PR ?

[DrSoulain]

So, If I understand correctly, you want to add a new keyword in select_clean_data() to check directly within the function instead of using another function show_clean_params().

In the near-future release, we added the CLI possibility to amical and included your suggestion.

But for now, I think the best way to add what you propose is to run show_clean_params() at the beginning of select_clean_data() using a show_param argument and return None within (to stop the cleaning process at this step). You don't want to clean the data if your parameters are not well placed.

[vandalt]

The way I was seeing this change, it would not replace the ability to run show_clean_params separately and check that parameters are OK before running further steps, which if I understand correctly is the use-case you are describing. It would be an addition to simply enable users to visualize their cleaning parameters right before the cleaning is done. So in summary:

• show_clean_params() still exists and can be used to check the parameters before cleaning
• select_clean_data() has an (optional) internal call to show_clean_params. The new keyword arguments are just to allow passing kwargs to show_clean_params

I think this addition can be useful for users who interact with the API directly (regardless of if the CLI does it) especially if they want to generate all possible plots when they clean the data. AMICAL is quite fast to run so I could see value in being able to generate the plots in a simple way, inspect them later, and re-run if needed (or just show them, stop the script with ctrl+c and re-run).

That being said, if the CLI update brings a similar API change. I suggest to make this a draft PR, wait for the CLI update, and see if these changes would still be relevant after the CLI is merged. Does that sound good @DrSoulain ?

[vandalt]

I converted this to a draft PR for now, I can rebase it if it is still useful after #76 is merged.

[vandalt]

I rebased this on the latest version and reorganized it a bit:

• There is now a check_clean_params kwarg in select_clean_data. It controls whether show_clean_params() is called from select_clean_data(). It is set to False by default so that the default behaviour of the code is unchanged.
• I used this new functionality in the SPHERE example, because in this case it is the same as calling show_clean_params() outside.
• In the CLI, the default behaviour is unchanged: the cleaning parameters are shown only with --check, outside of select_clean_data()

I think this add the functionality I was aiming to add at first, but with better backward compatibility and simpler defaults. I think it is ready for review now that #76 is merged.