Prefix containers #833

maxnoe · 2018-11-22T17:17:58Z

This allows writing multiple containers using a prefix for each column in the hdfwriter.

This enables writing duplicated keys (e.g. ReconstructedShower.alt, MCEvent.alt).

By default the prefix is cls.__name__.lower().replace('Container', '')

codecov · 2018-11-22T17:39:08Z

Codecov Report

Merging #833 into master will increase coverage by 0.06%.
The diff coverage is 98.16%.

@@            Coverage Diff             @@
##           master     #833      +/-   ##
==========================================
+ Coverage   73.52%   73.58%   +0.06%     
==========================================
  Files         211      211              
  Lines       12066    12102      +36     
==========================================
+ Hits         8871     8905      +34     
- Misses       3195     3197       +2

Impacted Files	Coverage Δ
ctapipe/io/tests/test_hdf5.py	`93.08% <100%> (+0.36%)`	⬆️
ctapipe/core/tests/test_container.py	`100% <100%> (ø)`	⬆️
ctapipe/io/hdf5tableio.py	`94.63% <100%> (+1.52%)`	⬆️
ctapipe/io/tableio.py	`90% <100%> (+0.16%)`	⬆️
ctapipe/io/containers.py	`100% <100%> (ø)`	⬆️
ctapipe/core/container.py	`67.32% <90%> (+0.32%)`	⬆️
ctapipe/io/nectarcameventsource.py	`95.23% <0%> (-4.77%)`	⬇️
ctapipe/io/lsteventsource.py	`87.02% <0%> (-1.53%)`	⬇️

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 e715248...405da47. Read the comment docs.

vuillaut

Very very useful feature, thank you @maxnoe.

vuillaut · 2018-11-23T10:00:18Z

The way I understand it, the user can change the prefix as he likes, right?

maxnoe · 2018-11-23T10:02:08Z

The way I understand it, the user can change the prefix as he likes, right?

Not really intended. The prefix is a class member of the container.
But as this is python, you could change this class member.

from ctapipe.io.containers import HillasParametersContainer

HillasParametersContainer.prefix = 'foobar'

ctapipe/core/container.py

dneise · 2018-11-26T13:56:34Z

ctapipe/io/hdf5tableio.py

+        group_name,
+        add_prefix=False,
+        mode='w',
+        root_uep='/',


what is a "uep" ?

"The user entry point to the object tree attached to the HDF5 file is represented in the root_uep attribute". From http://www.pytables.org/usersguide/libref/file_class.html

We want to get rid of root_uep. We want to merge it with the group_name.

kosack · 2018-11-26T15:36:27Z

The way I understand it, the user can change the prefix as he likes, right?

Not really intended. The prefix is a class member of the container.
But as this is python, you could change this class member.

This is definitely the behavior we need - the main use case is that we will have for example multiple image cleanings and multiple hillas parameterizations that will be stored in the output file, so we want to be able to change the prefix for each one. E.g. if you have multiple image cleanings, for example (loose and tight), you might have a dict inside your container like:

cont.hillas['tight'] = hillas_parameters(...)
cont.hillas['loose'] = hillas_parameters(...)

then you want to have outputs like:

tight_length, tight_width (or perhaps tight_hillas_width if you combine multiple prefixes)

I guess that functionality can be in addition to the one added here (both are useful - a global classname prefix, and an "instance" prefix)

ctapipe/io/hdf5tableio.py

maxnoe · 2018-12-04T14:03:45Z

I updated this to enable instance level prefixes and moved the build_prefix function to the base TableWriter.

Containers now have a class variable containter_prefix which is used to initialize the instance level prefix.

Unfortunately this was not possible differently, since python does not allow class variables to initiliaze __slots__.

@kosack can you review again?

codecov-io · 2018-12-12T14:53:32Z

Codecov Report

Merging #833 into master will increase coverage by 0.09%.
The diff coverage is 97.61%.

@@            Coverage Diff             @@
##           master     #833      +/-   ##
==========================================
+ Coverage   78.11%   78.21%   +0.09%     
==========================================
  Files         185      185              
  Lines       10557    10593      +36     
==========================================
+ Hits         8247     8285      +38     
+ Misses       2310     2308       -2

Impacted Files	Coverage Δ
ctapipe/io/tests/test_hdf5.py	`93.08% <100%> (+0.36%)`	⬆️
ctapipe/plotting/tests/test_camera.py	`100% <100%> (ø)`	⬆️
ctapipe/core/tests/test_container.py	`100% <100%> (ø)`	⬆️
ctapipe/io/hdf5tableio.py	`94.63% <100%> (+1.52%)`	⬆️
ctapipe/io/tableio.py	`90% <100%> (+0.16%)`	⬆️
ctapipe/io/containers.py	`100% <100%> (ø)`	⬆️
ctapipe/core/container.py	`67.32% <85.71%> (+0.32%)`	⬆️

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 bc12d6b...1f28b34. Read the comment docs.

ctapipe/io/tests/test_hdf5.py

kosack

just minor change - don't use tempdir, rather pytest's fixture instead. Otherwise looks good.

ctapipe/io/tests/test_hdf5.py

* master: Major coordinate refactoring, fixes cta-observatory#899, cta-observatory#900, cta-observatory#505, cta-observatory#416 (cta-observatory#896) speed up is_compatible; makes event_source factory faster (cta-observatory#927) Prefix containers (cta-observatory#833) remove optional deps in order to fix tests (cta-observatory#925) Charge Resolution Update (cta-observatory#827) Remove serializer, fixes cta-observatory#887 (cta-observatory#913) deleted summary.html - accidentally committed file (cta-observatory#919) # Conflicts: # ctapipe/tools/extract_charge_resolution.py # examples/simple_event_writer.py

vuillaut previously approved these changes Nov 23, 2018

View reviewed changes

dneise reviewed Nov 26, 2018

View reviewed changes

ctapipe/core/container.py Show resolved Hide resolved

dneise reviewed Nov 26, 2018

View reviewed changes

kosack approved these changes Nov 26, 2018

View reviewed changes

kosack requested changes Nov 28, 2018

View reviewed changes

ctapipe/io/hdf5tableio.py Outdated Show resolved Hide resolved

maxnoe added 3 commits December 4, 2018 14:21

Give containers a prefix attribute

c3bba51

Allow setting a prefix for container columns in hdfwriter

7da2375

Fix codacy issues

2351a33

maxnoe force-pushed the prefix_containers branch from 9e9c68c to 2351a33 Compare December 4, 2018 13:21

maxnoe added 2 commits December 4, 2018 14:27

Add failing test for instance level prefixes

a3c2f21

Add support for instance level container prefixes

16e1564

Move prefix builder to container.items

669ac07

maxnoe dismissed vuillaut’s stale review via 669ac07 December 11, 2018 11:33

maxnoe and others added 2 commits December 11, 2018 14:16

Use HessioEventSource in test

405da47

Merge branch 'master' into prefix_containers

8e9d6fb

kosack previously approved these changes Dec 12, 2018

View reviewed changes

Fix style issues

8b08f2d

maxnoe dismissed kosack’s stale review via 8b08f2d January 3, 2019 11:03

Merge remote-tracking branch 'upstream/master' into prefix_containers

057bc95

dneise reviewed Jan 4, 2019

View reviewed changes

ctapipe/io/tests/test_hdf5.py Outdated Show resolved Hide resolved

kosack requested changes Jan 8, 2019

View reviewed changes

ctapipe/io/tests/test_hdf5.py Show resolved Hide resolved

Dominik Neise added 2 commits January 18, 2019 14:01

remove some unneedd str conversion

2b40629

use pytest tmp_path instead of tempfile.NamedTemporaryFile -> @kosack

66b8674

dneise approved these changes Jan 18, 2019

View reviewed changes

kosack approved these changes Jan 18, 2019

View reviewed changes

kbruegge approved these changes Jan 19, 2019

View reviewed changes

Merge remote-tracking branch 'upstream/master' into prefix_containers

1f28b34

dneise merged commit c2efb64 into cta-observatory:master Jan 23, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prefix containers #833

Prefix containers #833

maxnoe commented Nov 22, 2018

codecov bot commented Nov 22, 2018 •

edited

Loading

vuillaut left a comment

vuillaut commented Nov 23, 2018

maxnoe commented Nov 23, 2018 •

edited

Loading

dneise Nov 26, 2018

thomasgas Dec 12, 2018

kbruegge Dec 12, 2018

kosack commented Nov 26, 2018 •

edited

Loading

maxnoe commented Dec 4, 2018 •

edited

Loading

codecov-io commented Dec 12, 2018 •

edited by codecov bot

Loading

kosack left a comment

Prefix containers #833

Prefix containers #833

Conversation

maxnoe commented Nov 22, 2018

codecov bot commented Nov 22, 2018 • edited Loading

Codecov Report

vuillaut left a comment

Choose a reason for hiding this comment

vuillaut commented Nov 23, 2018

maxnoe commented Nov 23, 2018 • edited Loading

dneise Nov 26, 2018

Choose a reason for hiding this comment

thomasgas Dec 12, 2018

Choose a reason for hiding this comment

kbruegge Dec 12, 2018

Choose a reason for hiding this comment

kosack commented Nov 26, 2018 • edited Loading

maxnoe commented Dec 4, 2018 • edited Loading

codecov-io commented Dec 12, 2018 • edited by codecov bot Loading

Codecov Report

kosack left a comment

Choose a reason for hiding this comment

codecov bot commented Nov 22, 2018 •

edited

Loading

maxnoe commented Nov 23, 2018 •

edited

Loading

kosack commented Nov 26, 2018 •

edited

Loading

maxnoe commented Dec 4, 2018 •

edited

Loading

codecov-io commented Dec 12, 2018 •

edited by codecov bot

Loading