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

Tally filters as first class objects #841

Closed
paulromano opened this issue Mar 22, 2017 · 4 comments
Closed

Tally filters as first class objects #841

paulromano opened this issue Mar 22, 2017 · 4 comments
Milestone

Comments

@paulromano
Copy link
Contributor

Right now, tally filters appear as their own independent objects in the Python API, but the dirty truth is that down at the XML and Fortran level, they really don't exist outside of a tally itself. Crucially, if you define a filter in the Python API and then use it in multiple tallies, it actually gets replicated. This means that at tally-time, these replicated filters get checked redundantly, which doesn't really make sense as @smharper has pointed out in #214. One of his suggestions was to separate tally filters from tally elements. For simulations with a lot of tallies that use the same filters (as often happens with MGXS generation), this may provide a substantial performance improvement. I've asked @amandalund to start looking at an implementation. While she's working on that, one thing I'd like to discuss/decide immediately is how we want to change the constructor for openmc.Filter. Right now it looks like:

filter = Filter(bins)

To make filters first class objects, they will need IDs (again, see #214 for @smharper's proposal). My question is:

Do we want to make __init__ consistent with other objects in the Python API (and break backwards compatibility)?

filter = Filter(1, bins)

or do we want to maintain backwards compatibility (but be inconsistent with the other objects which have IDs)?

filter = Filter(bins, 1)

I want to decide this now so that, if needed, we can at least change Filter.__init__ for version 0.9. Better to get these changes in now rather than after a release.

@paulromano paulromano added this to the v0.9 milestone Mar 22, 2017
@smharper
Copy link
Contributor

I would vote for the second option i.e.

filter = Filter(bins, 1)

because bins are a required positional argument, but the id number is an optional keyword argument.

Looking at it from that perspective, this option is not necessarily inconsistent with the other objects. It's just that the other objects are a degenerate cases where there are no positional arguments.

@paulromano
Copy link
Contributor Author

That is a compelling argument. I tend to agree. If we stick with that syntax, then we don't necessarily need to make any changes for v0.9. I'll leave the issue open for now though in case anyone else wants to disagree and make an argument.

@paulromano
Copy link
Contributor Author

Seeing no objections, I will consider the decision to be made that we'll stick with Filter(bins, id). We'll shoot for getting an actual implementation in v0.10.

@paulromano
Copy link
Contributor Author

All set with the merging of #871.

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

No branches or pull requests

3 participants