Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
ripsnet #587
base: master
Are you sure you want to change the base?
ripsnet #587
Changes from 21 commits
352fce1
51eeb03
f879d1d
8ff14bf
9eb3231
8e5003f
496a141
fcf7edf
e23e929
25b842e
f01e3cc
9281794
6b9ecce
ec19f2d
aef67bd
28d7320
32ba96b
33424c8
146b34e
f29ca81
6edf471
59dc256
db348e6
dd5e94e
25222ca
63edfc2
1cca679
1122d11
a25d3f8
a05f825
67cb4d4
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
It may be better to be explicit, i.e. naming the imported functions or to do
import gudhi.tensorflow as gtf
just to make clear to the user which functions below are indeed coming from thegudhi.tensorflow
package.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 agree, that makes it a bit clearer.
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.
Perhaps it may be worth to comment (not in detail) what are these hyper-parameters ; in particular how one is supposed to chose
ragged_layers_size
anddense_layers_size
.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.
Sure, I added a comment saying they should be tuned according to the specific dataset in order to reach a better performance.
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.
Uh, so we build data_test using numpy (from lists), only to convert it again to lists here, and finally build a tensorflow object from those lists? Would it be possible to skip some of those conversions? We probably don't need to import numpy at all.
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.
Yes, you're right, thanks. I updated it.
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.
Would it be cumbersome to provide a sort of minimal working example to train RipsNet ?
A user may not be familiar with tensorflow and have no clue on how to train the RipsNet model at this stage. Perhaps just setting the typical
optimizer = ...
,loss_function = ...
, and do a single step of gradient descent here, would help to and not discourage the user not familiar with tf?Another option is to write a Tutorial ( https://github.com/GUDHI/TDA-tutorial ) to reproduce, say, the synthetic experiment of the paper (multiple_circles) and to refer to it in this doc (I understand that we don't want this doc to be too long).
A final option (that requires more development) would be to provide a method
train
to RipsNet that does the job with some default parameters, so that one could get starting by simply going for something likeOf course these are just suggestions.
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 think the best option is to link to the notebook containing the synthetic examples. This provides a very nice example where one can see the workflow.
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've adapted Mathieu's original tutorial on the synthetic data so that it illustrates the use (including setup and training) of a RipsNet architecture. So, as @tlacombe suggested, I think it may be nice to include and link to this tutorial somewhere.
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've opened a PR (here: GUDHI/TDA-tutorial#59) to include the tutorial notebook so that we can then link to it.
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.
It isn't obvious what the example is computing. Maybe adding a comment or 2 would help (or an image). Is
data_test
a list of 3 point sets in 2D, and is the output some kind of vectorized persistence diagram? It becomes clear once we read the detailed doc, but I think some minimal comments in the example would still make sense.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.
Good point, I added a sentence to describe it.