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

mostly cosmetic, adding some type hints, and clarifying comments #16

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

Conversation

amirziai
Copy link

No description provided.

@@ -25,7 +25,9 @@ def __init__(self, batch=None, **kwargs):
self.__slices__ = None

@staticmethod
def collate(follow_batch=[], transform=None, **kwargs):
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this pattern appears in a few places i think:
https://docs.python-guide.org/writing/gotchas/#mutable-default-arguments

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi,

Thanks for proposing this. It seems that it is not a bug because we want the users to pass in a list for follow_batch and if they don't pass in the list we will make the follow_batch an empty list. We are not going to append elements to the follow_batch as appears in the link.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yea, def not a bug from what i can tell so it's minor. however, this pattern can lead to very tricky issues to debug and is generally advised against so wanted to point it out. no worries if you prefer it the way it is.

Copy link
Collaborator

@zechengz zechengz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi,

Thanks for your contribution! To approve your changes, I think you need to remove the nx.Graph type hints because we currently also support snapx graphs.

@amirziai
Copy link
Author

thanks @zechengz . i see, looks like snapx comes as a part of snap which is the pypi package snap-stanford?

what do you think about this:

GraphType = Union[nx.Graph, sx.Graph]

...

def __init__(self, G: Optional[GraphType] = None, netlib=None, **kwargs):

also again totally ok if you prefer it the way it is. adding the type hint makes it more clear for me when i'm reading the code but i understand if it's in the way.

@amirziai
Copy link
Author

amirziai commented Mar 2, 2021

@zechengz lmk if you like the suggestion above, otherwise i can drop the type params and we can merge this. thanks!

@zechengz
Copy link
Collaborator

zechengz commented Mar 8, 2021

thanks @zechengz . i see, looks like snapx comes as a part of snap which is the pypi package snap-stanford?

what do you think about this:

GraphType = Union[nx.Graph, sx.Graph]

...

def __init__(self, G: Optional[GraphType] = None, netlib=None, **kwargs):

also again totally ok if you prefer it the way it is. adding the type hint makes it more clear for me when i'm reading the code but i understand if it's in the way.

Sorry about the late reply. Thanks, it looks good to me. Yes, currently snapx is part of the the pypi package snap-stanford, which has a similar interface with the networkx. If you are interested, one example deepsnap supports snapx is https://github.com/snap-stanford/deepsnap/blob/master/examples/node_classification_cora.py

@amirziai
Copy link
Author

amirziai commented Mar 8, 2021

thanks @zechengz , just made the change. lmk if you have any other feedback i should consider.

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