Skip to content
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

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Conversation

parulv1
Copy link
Collaborator

@parulv1 parulv1 commented Jul 30, 2020

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

@@ -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
Copy link
Collaborator

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. =/

Copy link
Collaborator Author

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.

Copy link
Collaborator

@axiezai axiezai left a 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.

@@ -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
Copy link
Collaborator

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?

Copy link
Collaborator Author

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.

Copy link
Collaborator

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants