Fixes for groups after .set_ncomp()
#597
Conversation
michaeldeistler
force-pushed
the
groupfixes
branch
from
08590ff to
b8c9ba8
Compare
michaeldeistler
force-pushed
the
groupfixes
branch
from
ec85541 to
4154645
Compare
There was a problem hiding this comment.
I left a few comments. In general, while I see some upsides of this change, I am not super convinced of having the groups in nodes.
- It will make
nodesmore convoluted and add a bunch of boolean columns in some locations (possibly between inserted mechanisms). - There will also be no visual distinction of a
Nagroup andNachannel. The user would have to know that (w.o. looking intogroup_names). - I am not a big fan of many
x_namesattributes inModule. I think dictionaries are more helpful.
Two ideas:
-
replace the old
groupsDict with a dataframe of booleans -
attach the group booleans only when you need them for
set_ncomp. -
would keep
nodesclutter-free, while retaining all the functionality we'd need for et_ncomp` to work and makes groups a more distinct object.
Lemme know what you think.
| @@ -118,7 +118,7 @@ def __init__(self): | |||
| self.total_nbranches: int = 0 | |||
| self.nbranches_per_cell: List[int] = None | |||
|
|
|||
| self.groups = {} | |||
| self.group_names: List[str] = [] | |||
There was a problem hiding this comment.
I see the reasons for keeping track of group names outside of nodes, but I slightly prefer the old groups style dictionary.
| ) | ||
| if key in self.base.group_names: | ||
| inds = self.nodes.index[self.nodes[key]].to_numpy() | ||
| view = self.select(inds) |
There was a problem hiding this comment.
select can also take the boolean of self.nodes[key] directly :)
| @@ -1242,12 +1251,12 @@ def add_to_group(self, group_name: str): | |||
| Args: | |||
| group_name: The name of the group. | |||
| """ | |||
| if group_name not in self.base.groups: | |||
| self.base.groups[group_name] = self._nodes_in_view | |||
| if group_name not in self.base.group_names: | |||
There was a problem hiding this comment.
An issue with tracking groups in the nodes dataframe that could arise is that a group name conflicts with other channel / param names. Could we add a check that ensure groups can only be added if a column of that name does not exist already?
|
|
||
| cell.branch(1).add_to_group("exc") | ||
| cell.branch(0).set_ncomp(2) | ||
| assert len(cell.exc.nodes) == 6 # 2 from branch(0) and 4 from branch(1). |
There was a problem hiding this comment.
also might be worth testing if set_ncomp correctly raises if ngroups > 1 within module.
Closes #560
In #560, we identified that groups do not get updated automatically when
set_ncomp()is run. This PR fixes that by makinggroupsa first-class citizen: Group identities are listed as a boolean column in.nodesnow. With this, we need no further special treatment ofset_ncomp(). In addition, we can get rid of special treatment of groups in thegraphswc reader.