[BUG] fixed dep matrix reader for serpent V 2.2.1 #512
Conversation
|
@DanKotlyar let me know if this is okay |
|
Hi Andrea,
I am sorry for the late response. Busy week.
I will check over the weekend.
Best regards,
Dan
…-------------------------------------------------------------------------
Dan Kotlyar
Associate Professor
Georgia Institute of Technology
G.W.Woodruff School of Mechanical Engineering
Nuclear and Radiological Engineering Program
770 State St NW<https://maps.google.com/?q=770+State+St+NW&entry=gmail&source=g>, Atlanta, GA 30332
Boggs Bldg 3-73 | Phone: +1.404.385.5372
http://pwp.gatech.edu/core/<http://pwp.gatech.edu/core/>
From: Andrea Alfonsi - NuCube ***@***.***>
Sent: Tuesday, February 27, 2024 10:53 AM
To: CORE-GATECH-GROUP/serpent-tools ***@***.***>
Cc: Kotlyar, Dan ***@***.***>; Mention ***@***.***>
Subject: Re: [CORE-GATECH-GROUP/serpent-tools] [BUG] fixed dep matrix reader for serpent V 2.2.1 (PR #512)
@DanKotlyar<https://github.com/DanKotlyar> let me know if this is okay
—
Reply to this email directly, view it on GitHub<#512 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AGLDW7YVJ6IKTACG72SV7R3YVX6PDAVCNFSM6AAAAABDXMTZVCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSNRWHA3TINRSG4>.
You are receiving this because you were mentioned.Message ID: ***@***.***>
|
There was a problem hiding this comment.
Thanks for this PR @alfoa! Can you add a test that shows this capability is working as expected?
It looks like the test suite is failing, but not for reasons in this PR. Pytest recently removed some functionality we were using so that's a thing for us to fix.
| @@ -41,6 +41,8 @@ class DepmtxReader(BaseReader, SparseReaderMixin): | |||
| ---------- | |||
| deltaT: float | |||
| Length of depletion interval | |||
| flx: float | |||
There was a problem hiding this comment.
Would prefer to have the full variable name just to be more explicit. And it should be typed as optional float, right? Since for older versions of serpent that don't output this data, users should expect it to not always be a float.
| @@ -41,6 +41,8 @@ class DepmtxReader(BaseReader, SparseReaderMixin): | |||
| ---------- | |||
| deltaT: float | |||
| Length of depletion interval | |||
| flx: float | |||
| Homogenized flux | |||
There was a problem hiding this comment.
Can you add a comment about this not existing for versions of Serpent less than 2.2.1
| if "zeros" in line: | ||
| line = stream.readline() |
There was a problem hiding this comment.
Why is the zero line added? If that's also new that's one thing but I'm pretty sure this line existed in earlier versions of Serpent.
Related to the removal of the zero line down below?
|
|
||
|
|
||
| if "zeros" in line: | ||
| line = stream.readline() |
There was a problem hiding this comment.
Same comment on why removing the zeros line read
|
|
||
|
|
||
| if "zeros" in line: | ||
| line = stream.readline() | ||
|
|
There was a problem hiding this comment.
Same comment on the removal of the zeros line read
Update the note depmtrx reader to process output files from serpent version 2.2.1
Closes #511
This modification keeps backward compatibility.
Make sure you have read over the developer guide to ease
the review process. These include:
docs/contributors.rstInclude a thorough and concise overview of the changes, and why this PR is overall beneficial to the project.
Once the PR has been created and is nearing closure, make sure that serious changes, including deprecations and incompatible changes are documented in the changelog