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
Implement paper #33
base: master
Are you sure you want to change the base?
Implement paper #33
Conversation
@@ -3,7 +3,9 @@ | |||
coupling alpha (in laplacian) and the local coupling alpha = 1. """ | |||
|
|||
import numpy as np | |||
|
|||
import sys | |||
import matplotlib.pyplot as plt |
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.
Are these imports necessary? I don't see them being used. =/
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.
Thanks. I don't know what it went there. I am removing it now.
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 sort of confused by the naming, I don't see network_transfer_P
anywhere in this file, and I see that you went back and forth with it in a previous commit 3d0a233.
We should also avoid function names like network_transfer_P
, it's not intuitive. Same goes for network_transfer_local_alpha
, The names should be straightforward, and the detailed description should go into the block comment describing the function.
spectrome/forward/runforward.py
Outdated
@@ -1,47 +1,8 @@ | |||
""" running the ntf over a range of frequencies.""" | |||
from ..forward import network_transfer as nt | |||
from ..forward import network_transfer_P as nt |
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.
Does forward.network_transfer_P
exist?
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 I had it at some point just because I was playing with some stuff but removed it later on. I don't have network_transfer_P
in the final version that I sent you (right?). Do you want me to remove that commit from the history?
So network_transfer_local_alpha
is the naming convention you had. I think we should discuss on how we want to keep the names. I think we can remove network_transfer_function
and replace it with network_transfer_local_alpha
. If you like this idea, I will go ahead with it. Let me know.
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.
From the looks of the code from ..forward import network_transfer_P
is in the code of the pull request, if that function doesn't exist in forward
, what are we importing? Does nt
run in your local copy? The import should fail if forward
doesn't have the function.
Let's discuss naming in the zoom meeting.
Hey Bobby!
Here is the updated code. I did not remove any functions but only modified the .._local_alpha. I had earlier deleted the other functions. Sorry for the delay. I am a newbie in all of this. Will appreciate any feedback and let me know if something is wrong.
Parul