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

fix: limit number of particles using the eventinfo #103

Merged
merged 20 commits into from
Aug 29, 2022

Conversation

jhgoh
Copy link
Contributor

@jhgoh jhgoh commented Oct 31, 2021

read_lhe() tries to append to the list of particles in the event, raising error for some LHE output with optional fields. Another function read_lhe_with_attributes() is safe from this issue because it skips strings starting with '#'.
From the LHE standard, the number of particles is already given in the first line of the LHE event, which is also available in the eventinfo.nparticles, and any lines after nparticles should be treated as optional field.

Related issue: #102

@codecov
Copy link

codecov bot commented Apr 21, 2022

Codecov Report

Merging #103 (df72add) into master (48f528e) will increase coverage by 4.47%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #103      +/-   ##
==========================================
+ Coverage   60.28%   64.76%   +4.47%     
==========================================
  Files           2        2              
  Lines         209      210       +1     
  Branches       56       56              
==========================================
+ Hits          126      136      +10     
+ Misses         73       65       -8     
+ Partials       10        9       -1     
Flag Coverage Δ
unittests-3.10 63.33% <100.00%> (+4.48%) ⬆️
unittests-3.6 61.95% <100.00%> (+4.59%) ⬆️

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

Impacted Files Coverage Δ
src/pylhe/__init__.py 71.09% <100.00%> (+5.40%) ⬆️

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

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.

As @eduardo-rodrigues had pointed out in Issue #102, it would be good to cover this with tests. I understand that as this has gotten ignored for a long time that this might not be something @jhgoh has time to add now (totally understandable and reasonable given the delay), so if that's the case then once I'm through the next week or so I can come and add tests for all the PRs that are lacking them.

@eduardo-rodrigues
Copy link
Member

Hi @matthewfeickert, I actually tested locally that this MR is all good with a file containing those lines starting with #aMCatNLO, as I found a file online for that. And since all tests present work, this MR is good to go. At this point in time a new release is really in need, and this MR is outstanding since long. I would kindly suggest that a new big release is made and more tests are added for robustness. I'm happy to add the file I'm mentioning to scikit-hep-testdata for a follow-up MR. WDYT?

@matthewfeickert
Copy link
Member

I'm happy to add the file I'm mentioning to scikit-hep-testdata for a follow-up MR.

Yeah a PR that adds more test data to scikit-hep-testdata would be great.

eduardo-rodrigues added a commit to scikit-hep/scikit-hep-testdata that referenced this pull request May 10, 2022
eduardo-rodrigues added a commit to scikit-hep/scikit-hep-testdata that referenced this pull request May 10, 2022
eduardo-rodrigues added a commit to scikit-hep/scikit-hep-testdata that referenced this pull request May 10, 2022
@eduardo-rodrigues
Copy link
Member

I'm happy to add the file I'm mentioning to scikit-hep-testdata for a follow-up MR.

Yeah a PR that adds more test data to scikit-hep-testdata would be great.

@matthewfeickert, I have added a test based on the file I added to scikit-hep-testdata. This is ready to be merged IMHO. Are you happy at this stage? [Test coverage is something to improve but unrelated to this MR.]

@eduardo-rodrigues
Copy link
Member

Closes #102

@eduardo-rodrigues
Copy link
Member

Hello @matthewfeickert, @lukasheinrich, @kratsg, I have added the requested test some time ago. Anything else or this can go in :-)?

src/pylhe/__init__.py Show resolved Hide resolved
src/pylhe/__init__.py Show resolved Hide resolved
@eduardo-rodrigues eduardo-rodrigues force-pushed the fixReadLHE branch 2 times, most recently from 6e248a2 to 313705c Compare August 24, 2022 17:16
matthewfeickert and others added 5 commits August 25, 2022 09:34
* As https://github.com/zeke/semantic-pull-requests is no longer maintained
and is down, use the suggested alternative of
https://github.com/amannn/action-semantic-pull-request to check that PR titles
follow the Conventional Commits spec.
* Add graphviz>=0.12.0 as a core dependency.
* Add graphviz.Digrap graph object as .graph property to LHEFile.
   - Add support for IPython visualization.
* Add test for LHEEvent_graph.
…-hep#124)

* Change all explicit relative imports to absolute imports.
   - Provides clearer more explicit imports for new developers.
* Add absolufy-imports to pre-commit hooks
* Add tbump to 'develop' extra and remove bump2version.
* Add tbump.toml to configure tbump.
   - Allow for valid versions to follow SemVer and also support release
candidates: <major>.<minor>.<patch>rc<candidate>
* Remove .bumpversion.cfg.
matthewfeickert and others added 8 commits August 25, 2022 09:34
* Lukas Heinrich is a professor at Technical University of Munich as of March 2022.
* Matthew Feickert is a postdoc at University of Wisconsin-Madison as of June 2022.
* Add Eduardo Rodrigues to development team listing.
   - Update Zenodo citation metadata, recommended citation, and setup.cfg.
* Update pre-commit hooks:
   - github.com/asottile/pyupgrade: v2.32.0 → v2.32.1
* Add example to README that shows how to open an LHE file and interact with
the events.
* Update pre-commit hooks:
   - github.com/pre-commit/pre-commit-hooks: v4.2.0 → v4.3.0
   - github.com/asottile/pyupgrade: v2.31.1 → v2.34.0
   - github.com/psf/black: 22.3.0 → 22.6.0
@eduardo-rodrigues
Copy link
Member

eduardo-rodrigues commented Aug 25, 2022

Done the homework @matthewfeickert. I would be happy with an approval at this stage so that I merge :-).

Closes #102.

@eduardo-rodrigues
Copy link
Member

Morning @matthewfeickert, let's try and tick this off for the weekend. At this point is seems straightforward since I trivially dealt with your 2 remaining comments.

@eduardo-rodrigues
Copy link
Member

Am now merging for the first time in this repo :-). I'm sure I ticked all to-dos from the review.

@eduardo-rodrigues eduardo-rodrigues merged commit d84e325 into scikit-hep:master Aug 29, 2022
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.

3 participants