Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Tests #141

Merged
merged 33 commits into from
Sep 5, 2023
Merged

Tests #141

merged 33 commits into from
Sep 5, 2023

Conversation

Elisa-Visentin
Copy link
Collaborator

Here some test functions (for the io module): real data and MCs are processed from DL0s up to DL3s/IRFs and provided to the load functions. Still to be adapted to the 4LSTs updates and to Git workflows (and maybe to the ctapipe 0.19 upgrades); now they can be run 'locally'. Test files now in my workspace on the IT Container; they will then be moved to a location providing, e.g., wget downloads with Git secrets.

In the meanwhile...
Comments? suggestions? errors?
Somebody wants to try to run them? (Maybe I need to tell you how to do this...)

See also #137

STILL NOT TO BE MERGED (Maybe after the 4LSTs upgrades)

jsitarek
jsitarek reviewed Jul 6, 2023
View reviewed changes
magicctapipe/conftest.py Outdated Show resolved Hide resolved
jsitarek
jsitarek reviewed Jul 6, 2023
View reviewed changes
magicctapipe/conftest.py Outdated Show resolved Hide resolved
@Elisa-Visentin
Update conftest.py
b215f76 
directories creation (downloads)
jsitarek
jsitarek reviewed Jul 7, 2023
View reviewed changes
magicctapipe/io/tests/test_io.py Show resolved Hide resolved
magicctapipe/conftest.py Outdated Show resolved Hide resolved
@Elisa-Visentin
Copy link
Collaborator Author

Still failed tests?

Added MAGIC-only tests (separated from the joint analysis ones)
MC: << number of showers.

Added input variable to magic_calib_to_dl1.py: maximum number of showers processed -> still 13000 showers to be processed in case of joint analysis: maybe adding another variable to 'shrink' MAGIC files up to approx. 500/1000 showers?

Any other suggestion, error...?

@jsitarek
Copy link
Collaborator

now it works like a charm for me, no broken tests any more

@jsitarek
Copy link
Collaborator

Duration of individual tests (after all the files are downloaded):

306.54s setup    magicctapipe/io/tests/test_io.py::test_load_magic_dl1_data_files
151.37s setup    magicctapipe/io/tests/test_io.py::test_get_stereo_events_mc
47.05s setup    magicctapipe/io/tests/test_io.py::test_load_mc_dl2_data_file
27.61s setup    magicctapipe/io/tests/test_io.py::test_get_stereo_events_data
13.91s setup    magicctapipe/io/tests/test_io.py::test_index
8.61s setup    magicctapipe/io/tests/test_io.py::test_load_dl2_data_file
6.58s setup    magicctapipe/io/tests/test_io.py::test_load_irf_files
1.21s call     magicctapipe/io/tests/test_io.py::test_get_dl2_mean_avg
0.58s call     magicctapipe/io/tests/test_io.py::test_get_stereo_events_mc
0.57s call     magicctapipe/io/tests/test_io.py::test_get_stereo_events_mc_cut
0.36s call     magicctapipe/io/tests/test_io.py::test_load_mc_dl2_data_file
0.35s call     magicctapipe/io/tests/test_io.py::test_load_mc_dl2_data_file_cut
0.34s call     magicctapipe/io/tests/test_io.py::test_load_mc_dl2_data_file_opt
0.22s call     magicctapipe/io/tests/test_io.py::test_load_mc_dl2_data_file_exc
0.22s call     magicctapipe/io/tests/test_io.py::test_load_dl2_data_file
0.21s call     magicctapipe/io/tests/test_io.py::test_load_dl2_data_file_opt
0.21s call     magicctapipe/io/tests/test_io.py::test_load_dl2_data_file_cut
0.16s call     magicctapipe/io/tests/test_io.py::test_load_train_data_files_off
0.15s call     magicctapipe/io/tests/test_io.py::test_load_train_data_files_g
0.13s call     magicctapipe/io/tests/test_io.py::test_get_dl2_mean_exc
0.13s call     magicctapipe/io/tests/test_io.py::test_get_dl2_mean_real
0.12s call     magicctapipe/io/tests/test_io.py::test_load_train_data_files_p
0.12s call     magicctapipe/io/tests/test_io.py::test_get_dl2_mean_mc
0.11s call     magicctapipe/io/tests/test_io.py::test_load_lst_dl1_data_file
0.11s call     magicctapipe/io/tests/test_io.py::test_load_dl2_data_file_exc
0.10s call     magicctapipe/io/tests/test_io.py::test_load_magic_dl1_data_files
0.09s call     magicctapipe/io/tests/test_io.py::test_get_stereo_events_data
0.09s call     magicctapipe/io/tests/test_io.py::test_get_stereo_events_data_cut
0.04s teardown magicctapipe/io/tests/test_io.py::test_index
0.03s call     magicctapipe/io/tests/test_io.py::test_load_irf_files
0.02s setup    magicctapipe/io/tests/test_io.py::test_get_dl2_mean_avg
0.01s call     magicctapipe/io/tests/test_io.py::test_save_pandas_data_in_table

@aleberti
Copy link
Collaborator

is it normal that download takes ages? what is the total size of the files?

@Elisa-Visentin
Copy link
Collaborator Author

Elisa-Visentin commented Jul 11, 2023 via email

@Elisa-Visentin
Copy link
Collaborator Author

Elisa-Visentin commented Jul 11, 2023 via email

@aleberti
Copy link
Collaborator

Just tried now, and everything is fine except for test_load_train_data_files_off where it fails with:

gamma_stereo = (PosixPath('/tmp/pytest-of-aberti/pytest-2/DL1_gamma_train0'), PosixPath('/tmp/pytest-of-aberti/pytest-2/DL1_gamma_test0'))

    def test_load_train_data_files_off(gamma_stereo):
        """
        Check off-axis cut
        """
        events = load_train_data_files(
            str(gamma_stereo[0]), offaxis_min="0.2 deg", offaxis_max="0.5 deg"
        )
>       data = events["LST1_M2"]
E       KeyError: 'LST1_M2'

magicctapipe/io/tests/test_io.py:111: KeyError
------------------------------------------------------------ Captured log call -------------------------------------------------------------
INFO     magicctapipe.io.io:io.py:493 
The following DL1-stereo data files are found:
INFO     magicctapipe.io.io:io.py:498 /tmp/pytest-of-aberti/pytest-2/DL1_gamma_train0/dl1_stereo_gamma_zd_16.087deg_az_108.0deg_LST-1_MAGIC_run201.h5
INFO     magicctapipe.io.io:io.py:498 /tmp/pytest-of-aberti/pytest-2/DL1_gamma_train0/dl1_stereo_gamma_zd_16.087deg_az_108.0deg_LST-1_MAGIC_run301.h5
INFO     magicctapipe.io.io:io.py:133 
In total 7 stereo events are found:
INFO     magicctapipe.io.io:io.py:170     M1_M2 (type 0): 0 events (0.0%)
    LST1_M1 (type 1): 2 events (28.6%)
    LST1_M2 (type 2): 0 events (0.0%)
    LST1_M1_M2 (type 3): 5 events (71.4%)

By the way, I had to change the IP in conftest.py to cp02 to make the download work.

@jsitarek
Copy link
Collaborator

Thank you @aleberti !
Concerning the lack of M1+M2 events this might be plain statistics, because those events are more rare than the types with LST. But it would be good to have at least one to avoid a possible problem in the future. To do it without modifying the files we could change e.g. the intensity cut used in the test (either to lower value to have some events that did not survive earlier, or it might also help to set it to higher value if some even loses LST-1 image then. However we should then check that there is no issue on further tests.

@codecov
Copy link

codecov bot commented Aug 22, 2023 •
edited
Loading

Codecov Report

❗ No coverage uploaded for pull request base (master@1f27c60). Click here to learn what that means.
Patch has no changes to coverable lines.

Additional details and impacted files 
@@            Coverage Diff            @@
##             master     #141   +/-   ##
=========================================
  Coverage          ?   36.63%           
=========================================
  Files             ?       45           
  Lines             ?     5238           
  Branches          ?        0           
=========================================
  Hits              ?     1919           
  Misses            ?     3319           
  Partials          ?        0

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

@aleberti
Copy link
Collaborator

Thank you @aleberti ! Concerning the lack of M1+M2 events this might be plain statistics, because those events are more rare than the types with LST. But it would be good to have at least one to avoid a possible problem in the future. To do it without modifying the files we could change e.g. the intensity cut used in the test (either to lower value to have some events that did not survive earlier, or it might also help to set it to higher value if some even loses LST-1 image then. However we should then check that there is no issue on further tests.

ok, now tests are passing, but somehow I am a bit puzzled. The test that is failing for me, never failed in the CI tests here. What failed was the one loading IRF files, where a key called file_names is found, but I cannot reproduce it offline. I added it in the list of keys to be checked and of course the tests now are fine.

About the test failing for me, indeed changing the intensity cut can work. In order to make this easier, I will include the configuration files used in the tests directly in the repository, and not download them from the web server. It will make easier to do changes if in the future we change something in configuration files.

@aleberti
Copy link
Collaborator

Ok, implemented to have the configuration files in the repository. I tried a few intensity cuts (even simply >0), but nothing, I always get the same failing test offline. I wonder how it is possible that here it was not failing. Apart from that, everything else is ok. @jsitarek , @Elisa-Visentin , if you are ok with this, I would say it can be merged.

@aleberti aleberti marked this pull request as ready for review August 22, 2023 11:50
@Elisa-Visentin
Copy link
Collaborator Author

Elisa-Visentin commented Aug 22, 2023 via email

@aleberti
Copy link
Collaborator

No worries, I already added the needed key in the IRF test. The test failing for me (but only offline), is another one (see a few comments above), but it is not failing here in the CI.

@Elisa-Visentin
Copy link
Collaborator Author

As for the merging... the coincidence algorithm has been re-implemented since the creation of this branch, so some merge conflicts will have to be solved (and maybe the configuration files read by the tests will have to be changed a bit: 2 lines were added), but the tests should work. BTW, while implementing these tests I did not changed the MCP coincidence script, I only added some files and modified a bit the maigc_calib_to_dl1, not modified in the master since February...).
So, could be merged but config files to be slightly modified (I will do this now)

Elisa-Visentin and others added 3 commits August 22, 2023 14:28
@aleberti
Copy link
Collaborator

Easy to see if tests work in master: merge it in this branch. And indeed they work.

@aleberti aleberti requested a review from jsitarek August 23, 2023 05:47
@aleberti
Copy link
Collaborator

@jsitarek if you also are ok with the PR now, feel free to merge it!

@aleberti
Copy link
Collaborator

Tried to update the CI file with some updated stuff, but better to do it in another PR. So I reverted the changes. I will not touch this PR anymore :)

@aleberti aleberti mentioned this pull request Aug 29, 2023
Merged
@aleberti aleberti merged commit 074d205 into master Sep 5, 2023
4 checks passed
@aleberti aleberti deleted the tests branch September 5, 2023 15:25
@aleberti aleberti mentioned this pull request Sep 25, 2023
Merged
@aleberti aleberti added the enhancement New feature or request label Nov 2, 2023
Elisa-Visentin pushed a commit that referenced this pull request Sep 12, 2024
@aleberti
Merge pull request #141 from cta-observatory/tests
41b66b9 
Implementation of tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aleberti aleberti aleberti approved these changes

@jsitarek jsitarek Awaiting requested review from jsitarek

Assignees
No one assigned
Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Elisa-Visentin @jsitarek @aleberti