added new tests

[Elisa-Visentin]

Added new tests to check if output files have been produced in each of the analysis step (should solve the issue of no failed tests in case data are not adequately processed by the scripts). @aleberti could this be a solution or should I change/add other tests?

[Elisa-Visentin]

#173 should solve the doc issue, if ready to be merged (Seems ready to me), so that we can then work on this PR

[aleberti]

#173 is not a PR to fix docs, and actually it is not ready, it still does not do what I originally wanted. The problem you have is different. From the doc generation output:

/home/runner/work/magic-cta-pipe/magic-cta-pipe/magicctapipe/utils/functions.py:docstring of magicctapipe.utils.functions.transform_altaz_to_radec:20: WARNING: py:obj reference target not found: astropy.coordinates.angles.Longitude
/home/runner/work/magic-cta-pipe/magic-cta-pipe/magicctapipe/utils/functions.py:docstring of magicctapipe.utils.functions.transform_altaz_to_radec:35: WARNING: py:obj reference target not found: astropy.coordinates.angles.Latitude

so something probably was changed/corrected at astropy level. You just need to change astropy.coordinates.angles.Latitude with astropy.coordinates.Latitude and astropy.coordinates.angles.Longitude with astropy.coordinates.Longitude in utils/functions.py and the documentation should build fine.

[Elisa-Visentin]

I meant this:

packages/setuptools_scm/_integration/setuptools.py:30: RuntimeWarning: 
ERROR: setuptools==56.0.0 is used in combination with setuptools_scm>=8.x

Your build configuration is incomplete and previously worked by accident!
setuptools_scm requires setuptools>=61

BTW, in the meanwhile i will fix the astropy issue, thanks!

[aleberti]

That error is still not solved unfortunately in #173 . It is not critical, because the docs building is fine even if that error is present. I will try to make it work in the lstchain branch, maybe it is a problem of python version.

[Elisa-Visentin]

Anyway, I produced this PR to try to fix the issue we discovered about tests (with HaST and lstchain 0.10): some tests now should fail if no output file has been produced in a step (the older tests won't fail, but we cannot test, e.g., the DL2 script if we have no DL1 data)

[Elisa-Visentin]

the docs issue seems just a version 'inconsistency' in the setup (could be due to one or more dependencies that requires a too old setuptools version). Maybe the newer version could be forced (I thought your PR did this) or we can update everything we can update (new ctapipe -> new pyirf....) and see if it is automatically solved

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (47918ad) 76.33% compared to head (1f44406) 76.83%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #179      +/-   ##
==========================================
+ Coverage   76.33%   76.83%   +0.49%     
==========================================
  Files          21       21              
  Lines        2502     2556      +54     
==========================================
+ Hits         1910     1964      +54     
  Misses        592      592              

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

[aleberti]

@Elisa-Visentin I think I found a nicer way to do this (looking at how ctapipe handle this very same issue). Since the files are created by running subprocess.run(), one can store the result in a variable, which holds, among others, the return code of the process. One can assert if that value is equal to 0 (=process successful) before returning the output file. I forced magic_calib_to_dl1 to exit with a return code of 1 and the tests depending on those files where run and returned ERROR (different from FAILED, but was reported correctly).

An example:

@pytest.fixture(scope="session")
def M2_l1_monly(temp_DL1_M_monly, dl0_m2, config_monly):
   """
   Produce a DL1 file
   """

   process_run = subprocess.run(
       [
           "magic_calib_to_dl1",
           f"-i{str(dl0_m2[0])}",
           f"-o{str(temp_DL1_M_monly)}",
           f"-c{str(config_monly)}",
           f"-m{int(maxmonly)}",
           "--process-run",
       ]
   )

   assert process_run.returncode == 0
   return temp_DL1_M_monly

[aleberti]

This works well also when exceptions are raised, because python will then give the generic exit code 1.

[Elisa-Visentin]

@aleberti I implemented them as separate tests to try, in the near future, a little upgrade: skip all the tests that are non-sense because files have not been produced in a previous step (e.g., skip DL2 tests if no DL2 file exist). But maybe we can do this even with your version, but it would be really useful if the tests returns a failure instead of error (I'm playing a bit with pytest: a global variable and an 'if Failed' should let us save a lot of time)

[Elisa-Visentin]

@aleberti I tried to change a bit the structure to group tests and, in case of non-processed/deleted/moved files (so that we don't have all the files we should have), skip the related tests. Now only implemented for stereo MC tests in MAGIC-only analysis (just to try it). What about this?

BTW, we must check again the combo-types in this branch before the merge. It seems that something has been lost in the merge (But I will check this)

[aleberti]

Thanks @Elisa-Visentin . To understand better, with the change you made, the skipped tests will be included in the summary pytest creates at the end right? Then, will failures and errors be still shown? Looking at the documentation, when using -r, the default is fE (failures+errors). However now it is xXs. At this point, maybe we can just use -ra (see https://docs.pytest.org/en/6.2.x/usage.html#detailed-summary-report). By chance, do you have an example of how the change works if we have some files that were not created and therefore trigger the skipping of some test?

[Elisa-Visentin]

Hi Alessio, here I did not produce DL1 protons:


The test checking if there are two DL1 proton files in the due folder fails (F status) while the ones dependent on it (if stereo exixt and the class that run tests on stereo) are skipped (s status). If you mean in the last summary, yes, we can use -ra to see also a detail of the failed tests (Thanks!)

[aleberti]

Ok, then from my side I think that this PR is good. Let's discuss on wednesday how to proceed, but I think this PR can be merged already, unless you need to add anything else.

[Elisa-Visentin]

I have to 'duplicate' the same structure (classes + dependencies) for the other tests (both in test_io and in test_io_monly), but I will do this in few days if you agree on this new structure

[Elisa-Visentin]

Something strange happened again with combos in the master. I am trying to fix it now

[jsitarek]

I think some of the tests are unnecessarily repeated for MAGIC-only and can be safely removed.
I left a few more specific comments

[Elisa-Visentin]

Thank you Julian, I will check this. Just a minute because I am a bit lost in the combinations (there is still something wrong). BTW, thanks Alessio, but go to rest :)

[Elisa-Visentin]

Now the combinations are fixed (right?) and we can work on duplicates...

[Elisa-Visentin]

I'm working on all your comments and suggestions. Hopefully, tomorrow we will be ready for the merge and the new tagged version for the school

[aleberti]

@jsitarek can you quickly check the latest changes by Elisa? I think they are fine and reply to your comments. After that she can merge this and I can make the new release.

[jsitarek]

@aleberti - done
let's merge it