MCP cleanup

[Elisa-Visentin]

Here we can delete the scripts/functions/modules... that are no more used (maybe not even maintained). Feel 'free' to delete/undelete files.
It will be merged before the ctapipe 0.19 upgrade (so that we won't have to upgrade the files that we don't want to maintain). BTW, every file kept in the repository master should have to be up-to-date and should be maintained in the near future.

Maybe we can also delete the download_data.sh and the modules/functions imported only by the deleted scripts (please, before deleting modules/functions, check if they have been moved/renamed/modified in at least one of the open PRs: we could run into a lot of merge conflicts!), but we can discuss this.

[aleberti]

I removed some scripts and unused files. For those in the comparison directory, I need to check with the other PR from Elli. But I can do it in Elli's PR, so that this can already be merged (as far as I am concerned).

[Elisa-Visentin]

Some functions...(e.g., EventClassifierPandas in the reco module) are no more used. Should they be kept or should they be removed?

[aleberti]

mmmh indeed. I was using some tools to check dead code, but it seems they did not detect completely unused classes. I think the problem is that those classes are imported in some __init__.py file, so these codes think they are used at least once. In principle, event_processing.py can be deleted, it is not used anymore anywhere in the code.

[aleberti]

ok, so checking a bit manually I have the following list. "not used anywhere" means that the functions in that module are defined and used only in a __init__.py file.

magicctapipe.irfs.utils.py: not used anywhere --> THIS COULD BE DELETED, BUT MAYBE SOME FUNCTIONS ARE USEFUL
magicctapipe.reco.classifier_utils.py: not used anywhere --> SUGGESTED FOR DELETION
magicctapipe.reco.direction_utils.py: not used anywhere --> SUGGESTED FOR DELETION
magicctapipe.reco.energy_utils.py: not used anywhere --> SUGGESTED FOR DELETION
magicctapipe.reco.event_processing.py: not used anywhere --> SUGGESTED FOR DELETION
magicctapipe.reco.global_utils.py: not used anywhere --> SUGGESTED FOR DELETION
magicctapipe.reco.stereo.py: not used anywhere --> SUGGESTED FOR DELETION
magicctapipe.utils.filedir.py: used in classifier_utils, magicctapipe.irfs.utils --> SUGGESTED FOR DELETION
magicctapipe.utils.gti.py: not used anywhere --> MIGHT BE USEFUL LATER
magicctapipe.utils.plot.py: used in magicctapipe.irfs --> SUGGESTED FOR DELETION
magicctapipe.utils.tels.py: used in magicctapipe.reco.stereo.py, magicctapipe.reco.direction_utils.py --> SUGGESTED FOR DELETION
magicctapipe.utils.utils.py: used in magicctapipe.reco.classifier_utils.py, magicctapipe.utils.gti.py, magicctapipe.irfs, conftest.py --> SUGGESTED FOR DELETION (BUT NOT EVERYTHING)

@Elisa-Visentin @jsitarek can you go through the list and confirm that I can delete what is tagged as "SUGGESTED FOR DELETION"? About the plotting functions magicctapipe.irfs.utils.py I am not sure, maybe we can keep them. Actually we should have somewhere the scripts used for the plots in the MAGIC+LST1 paper, probably in a separate repository, like it was done for the LST1 one (https://github.com/cta-observatory/lst-crab-performance-paper-2023). @jsitarek , would this be possible?

[Elisa-Visentin]

Thank you Alessio, I'll check them today/tomorrow and let you know. As for the plotting functions, we could keep them (we can use them, e.g., in notebooks to check some data/models features, although now they are not used)

[Elisa-Visentin]

As for the suggested deletions in the reco module, I agree (old functions, which are no more useful or for which the dependencies can provide us a substitute [e.g., scikitlearn functions to evaluate the training features and 'quality']). As for the utils, ctapipe and yaml can provide us the needed i/o functions, so we can delete filedir.py. As for tels.py, these were 6-telescope functions: they are not needed in the 4-LSTs pipeline we are working on, and adapting them to the new ctapipe version would be a 'useless' effort, so I think we could delete them. As for plot.py, these functions are a matplotlib 'extension' to save/set up plots: I'm not sure about the default settings (i.e., fontsize=14 is not adequate for plots that must be used in slides/posters), maybe it would be easier to set plot-wise settings for each plot when needed (script or notebook), so I think that it could be deleted. But I need Julian's opinion too (you have more experience than me).

BTW, I could not work on this yesterday, sorry!

[aleberti]

If no one has more comments on this (@jsitarek ?), I would proceed with deleting according to the list above. I will move possibly useful functions (as discussed) in another place within the repo, so that they are excluded from the documentation generation.

[aleberti]

At the end, to make things simple, I deleted almost everything in the list, except for gti.py and two functions from utils.py (one moved to gti.py, the other to io.py). If we ever need something from the deleted files, we can go back in the git history (even if I am quite sure it will not happen, we would have used those scripts already if we ever needed them). Also, the comparison directory was deleted, it was deprecated. Looks to me that now we can set this as ready to review and merge it in the master.

[codecov]

Codecov Report

Attention: 3 lines in your changes are missing coverage. Please review.

Comparison is base (ddfdcf3) 43.10% compared to head (7513a91) 76.13%.
Report is 48 commits behind head on master.

Additional details and impacted files

@@             Coverage Diff             @@
##           master     #164       +/-   ##
===========================================
+ Coverage   43.10%   76.13%   +33.02%     
===========================================
  Files          47       21       -26     
  Lines        5644     2489     -3155     
===========================================
- Hits         2433     1895      -538     
+ Misses       3211      594     -2617     

Files	Coverage Δ
magicctapipe/__init__.py	100.00% <100.00%> (ø)
magicctapipe/conftest.py	99.74% <100.00%> (ø)
magicctapipe/image/__init__.py	100.00% <100.00%> (ø)
magicctapipe/image/calib.py	86.79% <100.00%> (ø)
magicctapipe/image/cleaning.py	46.75% <ø> (+0.21%)	⬆️
magicctapipe/image/leakage.py	17.64% <ø> (ø)
magicctapipe/image/tests/test_calib.py	100.00% <100.00%> (ø)
magicctapipe/io/__init__.py	100.00% <100.00%> (ø)
magicctapipe/io/gadf.py	98.43% <100.00%> (ø)
magicctapipe/io/tests/test_gadf.py	100.00% <100.00%> (ø)
... and 8 more

... and 14 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[aleberti]

Tests pass; I suggest to merge this asap so that we can continue with the other PRs.
PS: I did not remove the files in the data directory, which were used in the functions in plot.py. They can be useful for reference (probably to be updated, but not a problem of this PR).

[Elisa-Visentin]

I agree with this, thank you!!
PS: I will update the tests with the moved functions (in particular for the i/o module)