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

Add a graph learning module #57

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

Add a graph learning module #57

wants to merge 134 commits into from

Conversation

nperraud
Copy link
Collaborator

Add a small module to learn the graph from the signals.

From the user side, he can simply use the class LearnGraph.

naspert and others added 30 commits March 19, 2018 10:57
- do not use underscores in functions args
# Conflicts:
#	pygsp/graphs/nngraphs/nngraph.py
#	pygsp/tests/test_graphs.py
@nperraud
Copy link
Collaborator Author

Here the method is based on smoothness, but I am not sure that this does help for the name.
Another way might be that the classe LearnedGraph would take an argument to switch between learning methods. I would not consider a NNGraph as a learned graph.

@mdeff
Copy link
Collaborator

mdeff commented Nov 10, 2019

Better to keep one class per method (the separation is clear and the parameters would be messy otherwise).

Well, NNGraph also builds a graph from a set of features. The difference is in the assumptions the methods make about the features.

What about something like graphs.LearnFromSmoothSignals? Not sure about learn or infer, nor signals or features or data. (Alternative methods would go as graphs.Learn*.)

BTW, graphs.NNGraph should probably be renamed graphs.NearestNeighbors. Will do in #43.

@nperraud
Copy link
Collaborator Author

I think it should be LearnedFromSmoothSignals following your idea to not put verbs in class. I think we should keep the word Learned.
I am OK with it, but it is not great... Shall I rename the file LearnGraph.py to LearnedGraph.py in the spirit that we put all future LearnedGraph there?

@mdeff
Copy link
Collaborator

mdeff commented Nov 10, 2019

Let's think a bit more about the name. If we'll group them, you can name the module graphs/learned.py.

@kalofolias
Copy link
Collaborator

kalofolias commented Nov 11, 2019

Did not hear from @kalofolb , I should probably do it myself.

Hi guys,
I will do the tutorial! Using new account, didn't take check the old one and missed the conversation.

About the name: if we call it "LearnFromSmoothSignals" it doesn't separate between my method and the one of Dong etal. The separation comes from the regularisation of the degrees vector, so there could be a parameter "method" or "model" with choices between "l2_degrees" or "log_degrees" or similar. But as @mdeff said, then we have different sets of parameters... So how do we name mine versus he one of Dong etal? LearnFromSmoothSignalsLog vs LearnFromSmoothSignalsL2?

@mdeff
Copy link
Collaborator

mdeff commented Nov 11, 2019

Good to hear from you @kalofolias! Thanks for the tutorial.

Do you like LearnedFromSmoothSignals as a name? Any other idea? Are there settings where we'd prefer Dong et al. over this one? If not, we might as well not wonder about the difference. Will it be implemented? If not, we can think about differentiating the methods when it'll be.

@kalofolias
Copy link
Collaborator

Yeah, I think it's an ok name... I can't think of a better one myself. The settings where Dong et al performs better are spotty, but I might implement it for completeness and comparison reasons later.

@nperraud
Copy link
Collaborator Author

I have renamed the graph... But the import is a bit dirty in the __init__.py. To make it clean, I would need to make a subfolder I think.
@mdeff I think that you wanted to eventually remove the folder for the nngraphs. Let me know what you want me to do.
Also, do you want me to make the code more pep8?

@kalofolias
Copy link
Collaborator

Hi guys,
First tutorial is ready! I need permissions with this account to push. I also made some improvements to the code by @nperraud and fixed a bug (the distances in matrix Z have to be squared!).

@nperraud
Copy link
Collaborator Author

I cannot give you access... @mdeff , does he really need permission? If yes, can you add him?

@mdeff
Copy link
Collaborator

mdeff commented Jan 17, 2020

Thanks for your patience. @kalofolias, you should have received an invitation. Upon accepting, you should be able to push your changes to this branch. Thanks for the tutorial!

kalofolias and others added 2 commits March 29, 2020 21:31
Fixed bugs and improved compute_theta_bounds (geom_mean=True vs False, islist->is_sorted)
Added some documentation (utils._nearest_neighbor)
Allowed LearnedFromSmoothSignals to take manually an edge_mask
@nperraud
Copy link
Collaborator Author

Just found a small bug.
@kalofolias , please pull

@nperraud
Copy link
Collaborator Author

oh actually you found it as well sorry

@mdeff
Copy link
Collaborator

mdeff commented May 14, 2020

Is @kalofolias actively using this? Can we push his improvements and tutorial while we have it in our mind? Thanks :)

@mdeff
Copy link
Collaborator

mdeff commented May 14, 2020

@nperraud: why wasn't this bug caught by the unit tests? Can we add some to avoid further issues?

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

Successfully merging this pull request may close these issues.

6 participants