-
Notifications
You must be signed in to change notification settings - Fork 12
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
[WIP] fix __iter__ #329
[WIP] fix __iter__ #329
Conversation
michaeldeistler
commented
Apr 10, 2024
•
edited
Loading
edited
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the rationale behind this change?
@@ -1055,14 +1055,9 @@ def shape(self): | |||
|
|||
``` | |||
network.shape = (num_cells, num_branches, num_compartments) | |||
cell.shape = (num_branches, num_compartments) | |||
branch.shape = (num_compartments,) | |||
cell.shape = (1, num_branches, num_compartments) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not so sure if we should assign dim=1 to cell
if you technically have no way to index into it (it is the object being indexed into).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For consistency, net
would also have to have shape=(1,...), which would be odd imo.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be added to #43 when settled.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, sorry, I had not meant to request a review yet...
Anyways, __iter__
is broken right now, see the PR description. But let's just talk in person this is not an urgent PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Went over the recent PRs and thought I'd just do a pass of this one as well, even though you did not request it.
Alright sounds good.
a1b70d8
to
fddb19a
Compare
adressed by #447 |