MCP upgrades: semi-automatic scripts and update of the current scripts (1 LST -> 4 LSTs); Torino team

[ranieremenezes]

Checking the differences between the two branches

[aleberti]

Running pyflakes magicctapipe gives:

magicctapipe/scripts/lst1_magic/setting_up_config_and_dir.py:32:1: 'time' imported but unused
magicctapipe/scripts/lst1_magic/setting_up_config_and_dir.py:392:5: local variable 'MC_electrons' is assigned to but never used
magicctapipe/scripts/lst1_magic/setting_up_config_and_dir.py:393:5: local variable 'MC_helium' is assigned to but never used
magicctapipe/io/io.py:914:9: undefined name 'df_events'

please fix those and at least the pyflakes tests will pass.

[Elisa-Visentin]

Before you say this, We (I actually am the only responsible) have to reintroduce global variables TEL_NAMES or TEL_COMBINATIONS in 3 codes because, due to the PR splitting, this version is a 'hybrid' where the old (MAGIC+LST1) and the new (4 LSTs+MAGIC) version have been 'glued' at the DL1 stereo level. These variables (in RFs and DL1 to DL2) will disappear as soon as we merge the other PRs (RFs and DL1->DL3) and are adequately commented to allow us to remove them. To allow for an easier upgrade (i.e., deleting them) they are no more imported but copy-pasted where needed (BTW, if we define them in the io there should be some errors/crashes because they have the same name as the output of the telescope_combinations function)

[codecov]

Codecov Report

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

Comparison is base (670aa69) 36.63% compared to head (ddfdcf3) 43.10%.
Report is 2 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #154      +/-   ##
==========================================
+ Coverage   36.63%   43.10%   +6.47%     
==========================================
  Files          45       47       +2     
  Lines        5238     5644     +406     
==========================================
+ Hits         1919     2433     +514     
+ Misses       3319     3211     -108     

Files	Coverage Δ
magicctapipe/conftest.py	99.74% <100.00%> (+0.01%)	⬆️
magicctapipe/image/__init__.py	100.00% <100.00%> (ø)
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_io.py	100.00% <100.00%> (ø)
magicctapipe/io/tests/test_io_monly.py	100.00% <100.00%> (ø)
magicctapipe/reco/estimators.py	18.30% <100.00%> (ø)
...ctapipe/scripts/lst1_magic/lst1_magic_train_rfs.py	17.56% <100.00%> (+0.56%)	⬆️
magicctapipe/scripts/lst1_magic/merge_hdf_files.py	15.00% <100.00%> (ø)
... and 7 more

... and 1 file with indirect coverage changes

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

[gabemery]

Stopped at io.
Some comments on code structure and naming. Bigger issue is the error raising code. It does 6 independent checks but doesn't reflect it in the error.

[Elisa-Visentin]

Please, don't delete the branch after the merge. We still need it to upload and modify some scripts (RF->DL3) and to add the needed tests (new PRs will be opened)

[aleberti]

Please, don't delete the branch after the merge. We still need it to upload and modify some scripts (RF->DL3) and to add the needed tests (new PRs will be opened)

why not on new branches?

[Elisa-Visentin]

They are still an update of the current scripts towards 4 LSTs, just not pushed now to have a smaller PR. This way we will use only 3 branches (pipeline / IT scripts /tests) instead of at least 9. The fewer the number of local branches, the more easily we can work on them (otherwise we should delete local branches or clone again the repository...). So, if you don't have to close it, we can add the new scripts just after the merging and open a new PR from this same branch

[aleberti]

@Elisa-Visentin @ranieremenezes to have tests running again, please merge the master into this branch to get the latest changes. ci.yml has conflicts, because I slightly changed the options when the CI should run (i.e. for all pull requests, and also for pushes on master branch). This should also fix the problem for the Torino_IT_Container branch, I saw that also there actions are failing due to the numba version problem, which I also had.

[Elisa-Visentin]

Alessio, do you want us to run black or do you want to fix the code layout with your next PR (pre-commit)?

[aleberti]

No need to do it in this PR, I merge it now.