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

feat: Read LHE file version #139

Merged
merged 4 commits into from
Aug 29, 2022
Merged

feat: Read LHE file version #139

merged 4 commits into from
Aug 29, 2022

Conversation

eduardo-rodrigues
Copy link
Member

@eduardo-rodrigues eduardo-rodrigues commented Aug 29, 2022

Effectively the diff of old PR #132 that is messy and now largely redundant in what it fixes.

Closes #132.

Squash and Merge Commit Message

* Raise KeyError if weightgroup is missing attribute 'type' or 'name'.
* Read LHE file version into attribute "LHEVersion".

@codecov
Copy link

codecov bot commented Aug 29, 2022

Codecov Report

Base: 68.75% // Head: 68.07% // Decreases project coverage by -0.67% ⚠️

Coverage data is based on head (c8e4a04) compared to base (b6f6edd).
Patch coverage: 37.50% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #139      +/-   ##
==========================================
- Coverage   68.75%   68.07%   -0.68%     
==========================================
  Files           2        2              
  Lines         208      213       +5     
  Branches       56       57       +1     
==========================================
+ Hits          143      145       +2     
- Misses         58       61       +3     
  Partials        7        7              
Flag Coverage Δ
unittests-3.10 66.66% <25.00%> (-0.65%) ⬇️
unittests-3.6 65.38% <25.00%> (-0.63%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/pylhe/__init__.py 75.00% <37.50%> (-1.03%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@UCSC-EarlAlmazan
Copy link

Thanks @eduardo-rodrigues for cleaning up PR #132 here, though one fix was omitted.

In the new init.py file under def read_lhe, below line 294
eventinfo = LHEEventInfo.fromstring(eventdata)
should be
particle_objs = [LHEParticle.fromstring(p) for p in particles if not p.strip().startswith("#")]
to omit lines starting with "#" when iterating over particle_objs.

@eduardo-rodrigues
Copy link
Member Author

Thanks @eduardo-rodrigues for cleaning up PR #132 here, though one fix was omitted.

In the new init.py file under def read_lhe, below line 294 eventinfo = LHEEventInfo.fromstring(eventdata) should be particle_objs = [LHEParticle.fromstring(p) for p in particles if not p.strip().startswith("#")] to omit lines starting with "#" when iterating over particle_objs.

Thank you for your reply. I thought that is no longer needed because of the way that part of the code was rewritten, see master, but will check in any case.

@matthewfeickert matthewfeickert changed the title feat: read LHE file version feat: Read LHE file version Aug 29, 2022
Copy link
Member

@matthewfeickert matthewfeickert left a comment

Choose a reason for hiding this comment

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

@eduardo-rodrigues I'll let you follow up on

I thought that is no longer needed because of the way that part of the code was rewritten, see master, but will check in any case.

but this LGTM so if that's all good this is good to merge whenever you bring it out of draft.

@eduardo-rodrigues eduardo-rodrigues marked this pull request as ready for review August 29, 2022 20:24
@eduardo-rodrigues
Copy link
Member Author

Thank you both. I committed something important for this PR + little improvements in view of the future. We can then move forward and I will follow-up on the above.

@eduardo-rodrigues
Copy link
Member Author

@matthewfeickert, ready to merge if you're also happy. As said above, there will be stuff to improve towards better coverage, but that's general.

@eduardo-rodrigues
Copy link
Member Author

Oh, you had approved already. OK, merging, thanks :-).

@eduardo-rodrigues eduardo-rodrigues merged commit e7faf40 into master Aug 29, 2022
@eduardo-rodrigues eduardo-rodrigues deleted the eduardo-pr-132 branch August 29, 2022 21:28
@eduardo-rodrigues
Copy link
Member Author

Thanks @eduardo-rodrigues for cleaning up PR #132 here, though one fix was omitted.
In the new init.py file under def read_lhe, below line 294 eventinfo = LHEEventInfo.fromstring(eventdata) should be particle_objs = [LHEParticle.fromstring(p) for p in particles if not p.strip().startswith("#")] to omit lines starting with "#" when iterating over particle_objs.

Thank you for your reply. I thought that is no longer needed because of the way that part of the code was rewritten, see master, but will check in any case.

FYI @UCSC-EarlAlmazan indeed the new line particles = particles[: int(eventinfo.nparticles)] is what effectively filters out the lines starting with "#" since it knows exactly how many items (=lines) to read using eventinfo.nparticles.

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.

None yet

3 participants