Closed gtauzin closed 4 years ago
Check out this pull request on
Review Jupyter notebook visual diffs & provide feedback on notebooks.
Powered by ReviewNB
@lewtun there has been a change in the subdirectory structure in examples
. May I ask you to adapt the placement of any data files or images accordingly?
@ulupo happy to adapt to the new structure if you can explain it to me - what exactly has changed?
also the MNIST notebook is ready for your review - i'll flush the outputs before the final merge!
@lewtun never mind, I thought (from fickle memory) that a dataset had been added. But it's just images.
@lewtun I'd like to delegate this review to @ammedmar if he's still available and happy to do it. I'll give it a quick look just after.
@lewtun I'd like to delegate this review to @ammedmar if he's still available and happy to do it. I'll give it a quick look just after.
sounds good to me!
View / edit / reply to this conversation on ReviewNB
ammedmar commented on 2020-09-23T20:25:20Z ----------------------------------------------------------------
There is an issue with singular/plural.
I would suggest: it is convenient to use filtrations of cubical complexes instead of ...
or (better but maybe not explicit enough)
it is convenient to use filtered cubical complexes instead of ...
lewtun commented on 2020-09-25T08:15:26Z ----------------------------------------------------------------
Good catch! I opted for your first suggestion because "filtered" has a common meaning in data analysis that might confuse the lay reader
View / edit / reply to this conversation on ReviewNB
ammedmar commented on 2020-09-23T20:25:21Z ----------------------------------------------------------------
I would remove "achieve". Also another singuler/plural issue: every pixel has a single value so "pixel values" looks sub-optima to me.
lewtun commented on 2020-09-25T08:15:41Z ----------------------------------------------------------------
Done
View / edit / reply to this conversation on ReviewNB
ammedmar commented on 2020-09-23T20:25:22Z ----------------------------------------------------------------
Choose same center as figure from article... maybe being explicit about it, although it can be read in the code.
lewtun commented on 2020-09-25T08:16:09Z ----------------------------------------------------------------
I got rid of the comment and added a small sentence in the main text to clarify what center is chosen
View / edit / reply to this conversation on ReviewNB
ammedmar commented on 2020-09-23T20:25:22Z ----------------------------------------------------------------
I would say what the i-th sublevel set is. Maybe add something like: that contains all pixels with value less than the i-th smallest pixel value in the gray scale image.
lewtun commented on 2020-09-25T08:16:15Z ----------------------------------------------------------------
Done
View / edit / reply to this conversation on ReviewNB
ammedmar commented on 2020-09-23T20:25:23Z ----------------------------------------------------------------
Replace the second H_1 by H_0
lewtun commented on 2020-09-25T08:16:22Z ----------------------------------------------------------------
Good catch! Fixed
View / edit / reply to this conversation on ReviewNB
ammedmar commented on 2020-09-23T20:25:24Z ----------------------------------------------------------------
Maybe say a word about symmetrization along the diagonal. I would just add "and symmetrize along the main diagonal".
lewtun commented on 2020-09-25T08:16:30Z ----------------------------------------------------------------
Done
View / edit / reply to this conversation on ReviewNB
ammedmar commented on 2020-09-23T20:25:24Z ----------------------------------------------------------------
This is looking good in the nbreview tool. Please make sure it does look good in the real world.
lewtun commented on 2020-09-25T08:17:06Z ----------------------------------------------------------------
The true test will be when the docs are compiled - I'll double check it before the release!
Good catch! I opted for your first suggestion because "filtered" has a common meaning in data analysis that might confuse the lay reader
View entire conversation on ReviewNB
I got rid of the comment and added a small sentence in the main text to clarify what center is chosen
View entire conversation on ReviewNB
The true test will be when the docs are compiled - I'll double check it before the release!
View entire conversation on ReviewNB
@ammedmar i've made the changes according to your suggestions - please have a final look!
@ulupo i opted against doing fancy feature selection as you suggested, mainly because of time but also because random forests are quite robust to correlated features. in the future we could try to refactor this example to be a full-blown reproduction of @gtauzin's paper but for now i think if should be enough to help users get started with the images
API
Reference issues/PRs Reopening of PR #442.
Types of changes
Description Add the MNIST full-blown ML example
Checklist
flake8
to check my Python changes.pytest
to check this on Python tests.