TableLoader

[maxnoe]

I adapted this from the class in the dl1 benchmarks workshop, but changed the structure a bit.

I added options for what to load and for now a single method load_telescope_events(tel_id) to read / join all needed columns for one telescope.

Methods to come:

• loading only stereo data
• loading events for multiple tel_ids (as dict by type or just as single table).
• more?

Needs #1769

[codecov]

Codecov Report

Merging #1771 (7700d48) into master (63e0df4) will increase coverage by 0.03%.
The diff coverage is 93.73%.

@@            Coverage Diff             @@
##           master    #1771      +/-   ##
==========================================
+ Coverage   91.80%   91.84%   +0.03%     
==========================================
  Files         188      190       +2     
  Lines       14910    15237     +327     
==========================================
+ Hits        13688    13994     +306     
- Misses       1222     1243      +21     

Impacted Files	Coverage Δ
ctapipe/core/tool.py	93.22% <ø> (ø)
docs/conf.py	86.00% <ø> (ø)
ctapipe/instrument/telescope.py	86.48% <50.00%> (-10.07%)	⬇️
ctapipe/io/astropy_helpers.py	89.85% <68.18%> (-10.15%)	⬇️
ctapipe/conftest.py	94.48% <91.66%> (-0.94%)	⬇️
ctapipe/io/tableloader.py	95.23% <95.23%> (ø)
ctapipe/instrument/subarray.py	92.82% <100.00%> (+0.37%)	⬆️
ctapipe/instrument/tests/test_subarray.py	100.00% <100.00%> (ø)
ctapipe/io/__init__.py	100.00% <100.00%> (ø)
ctapipe/io/tests/test_table_loader.py	100.00% <100.00%> (ø)
... and 3 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 63e0df4...7700d48. Read the comment docs.

[HealthyPear]

Updates

I have improved the current PR with

• loading DL2 shower geometry
• refactoring to ensure loading of any single table and of every (almost, see below) combination
• added a generic call function that makes the usage blind to dataset split mode

(Current) Caveats

• loading of trigger + instrument alone cannot work for now because they refer to different things and have no column in common
• I read shower geometry by pre-pending the name of the reconstruction algorithm in case there is more than 1, perhaps in the case of only 1 it can be avoided

TODO

• check for missing unit-tests or testing cases
• improve documentation (docstrings and docs)

[maxnoe]

Ok, I came finally around to doing this. I think the code is much cleaner now, especially by itroducing the _allow_empty_join function, much of the complicated checking if a table is already there is gone.

There are now only three public methods:

read_subarray_events, read_telescope_events and read_telescope_events_by_type.

The latter two support the use cases of the previous many functions by having this List[Union[str, int, TelescopeDescription]] as argument.

[kosack]

Look good! Much simpler, and works in all my use cases.

[LukasNickel]

There are a lot of codacy complaints. Some are false positives, some are minor issues. I dont see any of them as severe, but want to bring up the topic before approving.

[kosack]

I somehow missed all the codacy complaints. I agree with @LukasNickel that most are minor, but a few should be addressed. It would be good to at least:

• document get_structure (it is a public function, exported from the module in __all__, and is really not clear at all what it does from the name)
• document read_subarray_events
• probably want a # pylint: disable= protected-access at the top of any files that uses low-level hdf5 commands (like _v_*, which give false-positive warnings)
• don't use the variable name tables (used a few times), since it shadows the tables module, and that could lead to bugs in the future
• either disable the warning, or document each test with at least a few words about what the test does: e.g.

def test_read_subarray_events(test_file_dl2):
     """ check that we get some expected columns when reading a file with DL2 """

• It's not clear to my why we suddenly are getting redefined-outer-name warnings in the tests
• also not sure why we get a List[TelescopeDescription] is not iterable warning here: (aren't lists iterable?)

valid_tel_types = {str(tel_type) for tel_type in self.telescope_types}

Edit: it's a @lazyproperty, so I guess that gives a false positive, since it looks like a function to the linter

[maxnoe]

@kosack I think I addressed most/all relevant codacy issues

[maxnoe]

Ok, now the docs should be fixed as well, at least with regard to table loader and processor tool, for the rest I opened #1786

[LukasNickel]

The indentation seems off here, tab vs spaces?