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

Bug in applyInputParams of multiple material classes #424

Closed
keckler opened this issue Sep 27, 2021 · 7 comments
Closed

Bug in applyInputParams of multiple material classes #424

keckler opened this issue Sep 27, 2021 · 7 comments
Assignees
Labels
bug Something is wrong: Highest Priority

Comments

@keckler
Copy link
Member

keckler commented Sep 27, 2021

The applyInputParams methods of multiple material classes contain a common bug when applying enrichments to particular isotopes/elements. Take, for example, adjusting the enrichment of B10 within the B4C class:

 def applyInputParams(
        self, B10_wt_frac=None, theoretical_density=None, *args, **kwargs
    ):
        if B10_wt_frac:
            # we can't just use the generic enrichment adjustment here because the
            # carbon has to change with enrich.
            self.adjustMassEnrichment(B10_wt_frac)

If the user intentionally specifies B10_wt_frac=0, as in they want their absorber to have only B11, the enrichment will not be adjusted because the conditional evaluates to False.

This same bug applies to other material classes, including UO2, and possibly others.

@youngmit
Copy link
Contributor

Looks like this just needs an is not None to disambiguate the falsiness of zero (implicit truth checks of numbers is a pretty big footgun, as this demonstrates).

@keckler would you mind filing a PR to fix?

@jakehader jakehader added the bug Something is wrong: Highest Priority label Sep 27, 2021
@john-science
Copy link
Member

Does this bug only apply in this one file? I see several material have the applyInputParams method.

@jakehader
Copy link
Member

It appears that it's only in one material but the issue may be because B4C has a different constructor. It would be nice if the API for the materials with these applyInputParams was consistent. Maybe in the past we had kwargs but broke them out into specific args? I don't remember the history off hand.

@keckler
Copy link
Member Author

keckler commented Sep 28, 2021

@jakehader I think it is more than just the one material. I just scanned through the material classes in the framework, and in addition to B4C, it looks like the following material classes suffer from the same bug. Let me know if you disagree, maybe you see something that I don't.

UO2
MOX
Sulfur
ThU

But I do agree that all the materials seem to apply the input parameters in different ways.

I'm happy to fix the materials that I identified. But if people think there is a larger issue to be addressed her (i.e. making all the materials consistent in their applyInputParams method), then we can figure that out separately, I'd suggest.

@jakehader
Copy link
Member

Hi @keckler you're probably right on the others. If you have a suggested fix for B4C that you could generalized then that would probably be worthwhile to look at.

@keckler
Copy link
Member Author

keckler commented Sep 28, 2021

PR added at #425

@john-science
Copy link
Member

PR Merged! Thanks @keckler!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something is wrong: Highest Priority
Projects
None yet
Development

No branches or pull requests

4 participants