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

Create a fresh ArrayEventContainer for each event in SimTelEventSource #1952

Merged
merged 1 commit into from
Jun 29, 2022

Conversation

maxnoe
Copy link
Member

@maxnoe maxnoe commented Jun 26, 2022

Seems to have no impact on performance:

This branch:

SimTelEventSource: 1010ev [00:46, 21.70ev/s]

Master:

SimTelEventSource: 1010ev [00:46, 21.66ev/s]

I was bitten by the reuse of the container again, this time it was DL2 information from the previous events not being cleared.

@maxnoe maxnoe requested review from kosack and LukasNickel June 26, 2022 16:12
kosack
kosack previously approved these changes Jun 26, 2022
Copy link
Contributor

@kosack kosack left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah if there is no performance impact, this is definitely a good idea. all there reuse was originally just for performance, but i have to admit i never measured it.

i’m considering even replacing the current Container implementation with a dataclass based one, with frozen attributes. I tried it and it seems to be must as fast and would prevent most accidental changes if we’re required to fill all containers at creation time

@codecov
Copy link

codecov bot commented Jun 26, 2022

Codecov Report

Merging #1952 (ebb90e8) into master (31a0fcc) will decrease coverage by 0.00%.
The diff coverage is 92.59%.

@@            Coverage Diff             @@
##           master    #1952      +/-   ##
==========================================
- Coverage   92.20%   92.20%   -0.01%     
==========================================
  Files         193      193              
  Lines       15216    15203      -13     
==========================================
- Hits        14030    14018      -12     
+ Misses       1186     1185       -1     
Impacted Files Coverage Δ
ctapipe/io/simteleventsource.py 95.47% <92.59%> (+0.16%) ⬆️

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 31a0fcc...ebb90e8. Read the comment docs.

@maxnoe
Copy link
Member Author

maxnoe commented Jun 26, 2022

i’m considering even replacing the current Container implementation with a dataclass based one,

That won't have all our features like prefixes / data types / help strings etc. dataclasses are a very powerful code generator, but in the end you get normal python classes with some fields. We have our own code generator here (that's essentially what the ContainerMeta class is, a code generator) that's I think better suited than starting over from dataclasses.

E.g. we have __slots__, dataclasses don't support that I think

LukasNickel
LukasNickel previously approved these changes Jun 27, 2022
Copy link
Member

@LukasNickel LukasNickel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is a good idea.
Avoids all the subtle bugs with reused information

nbiederbeck
nbiederbeck previously approved these changes Jun 27, 2022
@maxnoe maxnoe added this to the v0.16.0 milestone Jun 28, 2022
@vuillaut vuillaut merged commit 4a76a90 into master Jun 29, 2022
@maxnoe maxnoe deleted the new_container branch June 29, 2022 08:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants