Use pytest fixtures in unit tests instead of from_name methods

[maxnoe]

We are preparing a number of changes to the data model, including also the instrument model and dropping support for older versions.

This is very hard to do when the tests are relying on very old files on the data server accessed via from_name.

Here I replace all use in our unit tests of from_name with pytest fixtures, which are defined mostly in ctapipe/conftest so we can change them in a single location.

Also, most fixtures are updated to use prod5 data.

[review-notebook-app]

Check out this pull request on 

See visual diffs & provide feedback on Jupyter Notebooks.

---

Powered by ReviewNB

[kosack]

what's the reasoning for this?

[maxnoe]

@kosack As explained in #1978, I think these methods do more harm than good.

They make it very easy to make mistakes like loading the wrong instrument compared to the data being analyzed. I stopped counting the occurences I had to change code from doing from_name to reading the subarray from the input file / just using the one provided by the event source.

[maxnoe]

I think the use case for our unit tests is much better solved by defining common fixtures.

It reduced the code already by quite a lot. I could also imagine just doing the changes in our tests and keeping from_name for now, but the main thing is that it was abused far too often resulting in many errors.

[codecov]

Codecov Report

Merging #1985 (0580385) into master (93527bf) will decrease coverage by 0.01%.
The diff coverage is 99.33%.

@@            Coverage Diff             @@
##           master    #1985      +/-   ##
==========================================
- Coverage   92.33%   92.32%   -0.02%     
==========================================
  Files         193      193              
  Lines       15708    15725      +17     
==========================================
+ Hits        14504    14518      +14     
- Misses       1204     1207       +3     

Impacted Files	Coverage Δ
ctapipe/image/tests/test_hillas.py	98.90% <95.12%> (-1.10%)	⬇️
ctapipe/calib/camera/tests/test_flatfield.py	92.85% <100.00%> (+0.17%)	⬆️
ctapipe/calib/camera/tests/test_pedestals.py	100.00% <100.00%> (ø)
ctapipe/conftest.py	94.89% <100.00%> (+0.64%)	⬆️
ctapipe/core/tests/test_component.py	97.81% <100.00%> (-0.03%)	⬇️
ctapipe/image/hillas.py	97.01% <100.00%> (-1.50%)	⬇️
ctapipe/image/muon/tests/test_intensity_fit.py	98.00% <100.00%> (ø)
ctapipe/image/muon/tests/test_ring_fitter.py	100.00% <100.00%> (ø)
ctapipe/image/tests/test_cleaning.py	100.00% <100.00%> (ø)
ctapipe/image/tests/test_concentration.py	100.00% <100.00%> (+3.70%)	⬆️
... and 23 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 93527bf...0580385. Read the comment docs.

[maxnoe]

@kosack I removed the "remove from_name" part for now and only included the changes to the fixtures.

It would make it much easier to test the data model changes without constantly having to update files on the data server to include this.

[kosack]

This is nice in that we don't rely on the data format. However, what about tests to read old data formats? We should still have those (perhaps not now but after the 1.0 release, to ensure backward compatibility)

[maxnoe]

The point is, that we now do want do break backwards compatibilty.

For me this means that tests checking compatibility to old data should be added after we merge the current PRs changing the data model, make a release and first thing we add tests testing files pepduced with that release?

[maxnoe]

I would propose the following:

• Merge the remaining, data model changing PRs
• Make the 0.16.0 release
• Process test data files using the released version and add them to the data server in some meaningful structure
• Add (parametrized) tests checking we are still compatible with all data model versions we want to support (i.e. for now the current one for 0.16.0 and the currently implemented on, e.g.:

@pytest.fixture("session")
def prod5_dl1_file(tmp_dir_factory):
    # run ctapipe-process tool
    return output_path
    

@pytest.fixture("session", params=["v0.16.0", "current"])
def prod5_dl1_files(request, prod5_dl1_file):
    if request.param == "current":
        return prod5_dl1_file
    return get_dataset_path(f"prod5/{request.param}/test.dl1.h5")

[maxnoe]

@kosack @nbiederbeck @LukasNickel Please review again