Skip to content

Implement paper#33

Open
parulv1 wants to merge 7 commits into
Raj-Lab-UCSF:masterfrom
parulv1:ImplementPaper
Open

Implement paper#33
parulv1 wants to merge 7 commits into
Raj-Lab-UCSF:masterfrom
parulv1:ImplementPaper

Conversation

@parulv1
Copy link
Copy Markdown
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

import numpy as np

import sys
import matplotlib.pyplot as plt
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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.

Comment thread 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
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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.

2 participants