-
Notifications
You must be signed in to change notification settings - Fork 51
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
[MRG] Reorganize creating coordinates. #122
[MRG] Reorganize creating coordinates. #122
Conversation
d56da42
to
b122ab1
Compare
b122ab1
to
8216fe2
Compare
8216fe2
to
be05d96
Compare
hnn_core/network.py
Outdated
n_common_feeds : int | ||
The number of common feeds. | ||
p_unique_keys : list of str | ||
The keys of the dictionary p_unique. |
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.
So far we have operated under the assumption that there is only one type of common feed: rhythmic. For the sake of consistency, we may want to treat common feeds the same way we treat unique keys, with the exception that common feeds will always be assigned to a position at the origin.
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.
humm ... okay so how does this relate to the PR here? sorry I am a bit lost. Note unique feeds are also assigned to a position in origin.
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.
Right now, we enter common feeds by supplying n_common_feeds
. There is nothing intrinsically wrong with this, however, I think it would be clearer if instead we supplied the argument p_common_keys
so that the pos_dict items associated with common feeds have keys that specify the type of common feed (e.g., tonic, rhythmic, etc.)
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.
@jasmainak what do you think about using an input arg similar to p_unique_keys
except for the common feeds?
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.
sorry I had missed that previous comment 3 days ago. Actually I like that the common feed has a single key because the position will be the same no matter what the spike pattern of the feed is like. But I agree, there is an inconsistency between unique and common. We should try to fix this everywhere in the codebase as part of a single PR.
what's your opinion on the plot? should I remove the feeds? or how should I fix it?
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 guess I don't understand why unique feeds can't then be input the same way (i.e., with just a number to specify the amount of unique feeds) since they are all assigned to the same location anyway.
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's your opinion on the plot? should I remove the feeds? or how should I fix it?
See below for my thoughts on the plot.
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.
We can probably change it, but we need to change it deep inside the code as in here: https://github.com/jonescompneurolab/hnn-core/blob/master/hnn_core/pyramidal.py#L948. The fact that class attributes are used instead of explicitly passed arguments makes the code very tangled and inter-connected :( I opened an issue #123 to investigate this separately
fa3c40f
to
18430ce
Compare
I think HNN-GUI did have some sort of model visualization. I'm not sure I understand why the position of all feeds is the same. From a big-picture model visualization perspective, I would expect the location of distal feeds to be different from the location of proximal feeds. Or is the distance from the granular (proximal) vs. superficial (distal) artificial cells defined somewhere else? If so, we may want to incorporate this into the model visualization somehow so that it doesn't confuse others. |
Hi - I really like this plot.. and would like to ad it too our website but I wonder why all the feeds are going to the center? Best, steph
*******************************************
Stephanie R. Jones, PhD
Associate Professor
Brown University
Department of Neuroscience
Stephanie_Jones@Brown.edu
https://sites.brown.edu/stephanie-r-jones-lab/
Human Neocortical Neurosolver
https://hnn.brown.edu <https://hnn.brown.edu/>
… On Jul 24, 2020, at 6:22 PM, Ryan Thorpe ***@***.***> wrote:
while I was at it, I also added a tiny method to plot the cell locations. I always wanted to visualize it. Maybe HNN-GUI already has something of this sort?
I think HNN-GUI did have some sort of model visualization.
<https://user-images.githubusercontent.com/15852194/88426659-91fdd300-cdbf-11ea-8c15-0b239972164e.png>
I'm not sure I understand why the position of all feeds is the same. From a big-picture model visualization perspective, I would expect the location of distal feeds to be different from the location of proximal feeds. Or is the distance from the granular (proximal) vs. superficial (distal) artificial cells defined somewhere else? If so, we may want to incorporate this into the model visualization somehow so that it doesn't confuse others.
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub <#122 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AEX6KSPMFFZTM66G6JGN2HDR5ICRHANCNFSM4PG3V44A>.
|
It's a good point, I am trying to understand this too. From what I understand the inputs can be in an arbitrary point in space. It depends where they are synaptically connected that defines whether they are distal or proximal etc. This diagram does not show synaptic connections. Do you have any suggestions to fix this? I could get rid of the inputs if that makes it clearer. |
My vote would be to remove the feed locations from this plot for now. Since feeds are modeled as artificial cells (i.e., with arbitrary location), it seems fair that our plots would either omit them from a plot of modeled cell locations or visualize the feeds as we have done previously in in the tutorials. The latter option we can be added to hnn-core later if desired. |
Feel free to merge once the CIs come green! |
This PR achieves few improvements:
pos_dict
in only place not multiple methodsxrange
xrange
toxxrange
sincexrange
is a Python builtin_create_coords
out of the class so it is explicit what goes in for this computation and what comes out (Zen of Python says: "Explicit is better than implicit.")Network
class. No morezdiff
attribute andgridpyr
attributecc @rythorpe @blakecaldwell