-
Notifications
You must be signed in to change notification settings - Fork 22
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: Add LHE writing functionality #233
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #233 +/- ##
==========================================
- Coverage 91.48% 91.17% -0.32%
==========================================
Files 2 2
Lines 235 306 +71
Branches 54 78 +24
==========================================
+ Hits 215 279 +64
- Misses 16 17 +1
- Partials 4 10 +6
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
7bfd160
to
ef0376f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @APN-Pucky. I'm going to say LGTM as it passes CI.
Maybe @eduardo-rodrigues can give it a look over too?
@APN-Pucky can you please add a summary commit message to the PR body that can be copy-pasted in for the squash and merge?
Done. |
Thanks @APN-Pucky! Can you now rebase this off of |
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
✔️ |
Thanks @APN-Pucky. I'll leave this open for @eduardo-rodrigues to look at, but if he hasn't reviewed it by Friday 2024-08-22 I'll merge this. We can then get a minor release out to PyPI as @agbuckley asked about in Discussion #238. |
Thanks!! |
Hi everyone. As you could guess I had been away and offline. This is a really nice addition :+1! My only comment to you @APN-Pucky is that the functionality should be visible as an example notebook for sure, probably in a dedicated new notebook (otherwise many will not realise writing is now available). |
I decided to add a
tolhe()
function to every class (since it is neither xml nor a simple string).The format strings can maybe still be improved?
read_lhe_init()
now returns aLHEInit
object combining weights, procinfo and initinfo. For backwards compatibility I added the get/set there so thatLHEInit
will still behave as previousLHEInit
, what is nowLHEInitInfo
.The tests can probably still be extended, but for the first draft I just hardcoded them for one file.
Already includes #232 and #231
Closes: #229
Squash Commit summary:
feat: Add LHE writing functionality