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.

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

allow enums in containers and support in tableio #978

Merged
merged 2 commits into from
Mar 27, 2019

Conversation

dneise
Copy link
Member

@dneise dneise commented Feb 20, 2019

Connected slightly to #972

just, in case we want to use enum.Enum somewhere I thought we might want to be able to serialize them as well.

Not sure this is the best way to do it. I've read somewhere that HDF5 natively supports storing enumeration types, so that it deals with the back and forth trafo from the numeric value to the actual enum instance. But for the moment I did this in ctapipes own io layer.

@dneise
Copy link
Member Author

dneise commented Feb 20, 2019

Codacy is just complaining about invalid syntax again -> f-strings. Apart from that, I think it might be fine.

@maxnoe
Copy link
Member

maxnoe commented Feb 20, 2019

There is a class IntEnum in enum. Might this obliviate the need to write custom code to write it out?

@dneise
Copy link
Member Author

dneise commented Feb 21, 2019

Might this obliviate the need to write custom code to write it out?

Might ... but I think converting back into an IntEnum upon reading is impossible without storing the enum class in the meta data. And this must be done upon writing it, no? So what is the point?

@maxnoe
Copy link
Member

maxnoe commented Feb 21, 2019

Ah sure.

@dneise dneise mentioned this pull request Feb 22, 2019
@codecov
Copy link

codecov bot commented Feb 25, 2019

Codecov Report

Merging #978 into master will increase coverage by 2.92%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #978      +/-   ##
==========================================
+ Coverage   80.44%   83.37%   +2.92%     
==========================================
  Files         193      189       -4     
  Lines       11051    10700     -351     
==========================================
+ Hits         8890     8921      +31     
+ Misses       2161     1779     -382
Impacted Files Coverage Δ
ctapipe/io/hdf5tableio.py 95.09% <100%> (+0.46%) ⬆️
ctapipe/io/tests/test_hdf5.py 94.55% <100%> (+1.47%) ⬆️
ctapipe/io/tests/test_available_sources.py 100% <0%> (ø) ⬆️
ctapipe/instrument/tests/test_camera.py 100% <0%> (ø) ⬆️
ctapipe/io/containers.py 100% <0%> (ø) ⬆️
ctapipe/io/__init__.py 100% <0%> (ø) ⬆️
ctapipe/core/component.py 96.29% <0%> (ø) ⬆️
ctapipe/io/sst1meventsource.py
ctapipe/io/tests/test_sst1meventsource.py
ctapipe/io/tests/test_targetio_event_source.py
... and 4 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 6a6230a...02b2c43. Read the comment docs.

@kosack
Copy link
Contributor

kosack commented Feb 27, 2019

Codacy is just complaining about invalid syntax again -> f-strings. Apart from that, I think it might be fine.

I contacted them and got the response that they do not yet support python 3.6+ (!!) but are looking into it. So generally we can ignore those errors. I thought I had disabled them in fact, but perhaps it has to be done for each branch?

kosack
kosack previously approved these changes Feb 27, 2019
@dneise dneise force-pushed the allow_enums_in_table_writer_and_reader branch from d27f533 to c25ca51 Compare March 15, 2019 10:58
@dneise
Copy link
Member Author

dneise commented Mar 22, 2019

@cta-observatory/cta-devs I think this might be mergeble

@dneise dneise merged commit cdc0c45 into master Mar 27, 2019
@dneise dneise deleted the allow_enums_in_table_writer_and_reader branch March 27, 2019 10:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants