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

Fix ParticleFilter to work with set inputs #3092

Merged
merged 2 commits into from
Jul 30, 2024

Conversation

johvincau
Copy link
Contributor

Description

In #2964, it was pointed out that openmc.ParticleFilter, despite its documentation permitting sets (more broadly Iterables), throws a TypeError when given one. This PR fixes this issue.

The specific issue is that the attribute setter calls the function np.atleast_1d() on the input, which does not register sets as one-dimensional (and rightfully so, since sets don't have dimensions that can be indexed).

This function causes a set to be wrapped in a list, which causes issues with cv.check_iterable_type() since the depth of the iterable is increased beyond 1.

Quick Fix

Allowing set inputs for ParticleFilter seems uniquely appropriate, given its requirement for unique elements, and the rest of the code works fine with any iterable, set or not. In any case, only minor tweaks are required to make sets work.

I've opted to simply avoid calling np.atleast_1d() on sets, and use cv.check_type() which has extra provisions for checking if an Iterable contains a certain type in lieu of cv.check_iterable_type().

Side Issue

Despite its name, cv.check_iterable_type() only works on Sequences since it traverses the input with indexing. A more accurate name would probably be cv.check_sequence_type(), but then every other instance of this function must also be changed to reflect this.

Checklist

  • I have performed a self-review of my own code
  • I have run clang-format (version 15) on any C++ source files (if applicable)
  • I have followed the style guidelines for Python source files (if applicable)
  • I have made corresponding changes to the documentation (if applicable)
  • I have added tests that prove my fix is effective or that my feature works (if applicable)

Copy link
Contributor

@paulromano paulromano left a comment

Choose a reason for hiding this comment

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

I think the problem here is really that we should not be allowing any iterable object but rather we should only be accepting sequences (i.e., that can be indexed). With your patch here, the following does not work:

f1 = openmc.ParticleFilter({'photon', 'neutron'})
f1.bins[0]

Can you update this so that we only accept sequences?

Copy link
Contributor

@paulromano paulromano left a comment

Choose a reason for hiding this comment

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

Thanks @johvincau!

@paulromano paulromano merged commit 467b8e9 into openmc-dev:develop Jul 30, 2024
16 checks passed
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.

None yet

2 participants