Cleanup of determineTargetComponent #1230
Conversation
- now checks type in addition to flags if flags doesn't produce a target component - and is smart enough to set teh target component when there's a single solid component - overhauled and improved the unit testing
albeanth
added
enhancement
| componentWFlag = [ | ||
| c for c in b if (c.p.type in b.p.type or b.p.type in c.p.type) | ||
| ] |
There was a problem hiding this comment.
I'm not sure I understand what the underlying assumption is here that makes this a valid thing to do. Basically you're saying that if a component in the block has the same name as the block itself then it should be the target component?
There was a problem hiding this comment.
It might be good to consider components by name since there is no guarantee that this will work for all block types and might be fragile. Is it worth considering a refactor for always using a component name or first flags and then name?
There was a problem hiding this comment.
@jakehader Can you elaborate on why you think checking block types is fragile? Or more fragile than checking names?
We've had good success (I think) using the flags-based approach, so I would advocate that we keep using that as the primary methodology to obtain the axial expansion target component. Adding a check on type was recently requested to enable the axial expansion changer to work for cases when there is a component defined in the blueprints that does not have a flag registered for it and where there was not a desire to modify the source code to register a new flag.
I'll tag you and @keckler where the corresponding unit test is.
There was a problem hiding this comment.
After some offline discussions, this type check was rolled back in d9d138b.
| For the case in which a block contains a component that does not have Flags defined. | ||
| So instead of matching on flags, we match on type. | ||
| """ | ||
| b = HexBlock("detector", height=10.0) |
There was a problem hiding this comment.
@keckler @jakehader Here is the unit test that leverages the type checking.
This block, "detector", contains a "detector" component. Since "detector" is not a registered flag the flag-based approach won't work. So our options are to either 1) modify source code to add a new flag, or 2) add a new capability to infer target components based on something other than flags. I got feedback that option 2 was more desirable and that checking types could be an option.
The logic for c.p.type in b.p.type or b.p.type in c.p.type is to try and accommodate the case where the strings that define the component and block types aren't exactly the same. In this test, the component type is detector neutron and the block type is neutron. In this case the component type isn't "in" the block type, but the block type is "in" the component type. If the types were reversed, the opposite would be true (i.e., the component type would be "in" the block type).
Hopefully that's not as clear as mud...
There was a problem hiding this comment.
Isn't there a third option? To specify the target component on the blueprints using axial expansion target component? I'm just kinda confused why this change is necessary in the first place, but maybe I'm missing something.
There was a problem hiding this comment.
Yeah, that is a third, and very valid, option.
This PR is in response to a series of requests to not have to use that blueprints attribute.
There was a problem hiding this comment.
After some offline discussions, this type check was rolled back in d9d138b.
albeanth
changed the title
|
This PR is ready, but I am converting to draft while we figure out downstream merge dependencies. |
Description
This PR brings in various improvements to axialExpansionChanger.py::ExpansionData::determineTargetComponent.
if Flags do not produce a target component, the component and block types are now checked. This is useful for the case where you have a target component that does not have a flag defined.Rolled this back and users should use the block attribute
axial expansion target componentintroduced in Enable Users To Explicitly Specify Target Component for Axial Expansion #777 .Checklist
doc/release/0.X.rst) are up-to-date with any bug fixes or new features.docfolder.setup.py.