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

extractSerpent2GCs.py fix #100

Merged
merged 3 commits into from
Nov 14, 2019
Merged

Conversation

smpark7
Copy link
Collaborator

@smpark7 smpark7 commented Nov 12, 2019

The latest change by @gridley introduced automatic checking for the number of DNP groups (6 or 8 depending on the nuclear data library) to remove extra zeros if there are only 6 groups.

The script works for cases with secondary branches (resulting from Serpent automated burnup sequences or varying control rod positions)

The script fails for cases without secondary branches as the "branch" variable is null. The fix simply involves removing the "branch" variable call for these cases in the DNP group check.

The if-else code portions are also swapped to maintain consistency with the original code.

@smpark7 smpark7 requested a review from gridley November 12, 2019 18:05
@gridley
Copy link
Collaborator

gridley commented Nov 12, 2019

Ah, OK, this makes sense. Interesting that this error was in there for quite a while!

I will request a single change: could you change it so that rather than saying if (not secBranch) else , it instead says if (secBranch) else ? It is more easy to read then. Aside from that, the fix looks good.

@smpark7
Copy link
Collaborator Author

smpark7 commented Nov 12, 2019

Sure I'll swap them around

@pep8speaks
Copy link

pep8speaks commented Nov 12, 2019

Hello @smpark7! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2019-11-14 19:23:13 UTC

@gridley
Copy link
Collaborator

gridley commented Nov 12, 2019

Well, yep, once the PEP8 stuff is addressed I'll merge.

Copy link
Collaborator

@gridley gridley left a comment

Choose a reason for hiding this comment

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

Looks allll good!

@smpark7 smpark7 merged commit 4a6c788 into arfc:devel Nov 14, 2019
@smpark7
Copy link
Collaborator Author

smpark7 commented Nov 14, 2019

Thanks!

@smpark7 smpark7 deleted the fix-extract-serpent-gc branch November 14, 2019 20:27
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