The Wayback Machine - https://web.archive.org/web/20220218122125/https://github.com/scikit-learn/scikit-learn/pull/18964
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

Jump to bottom

FIX take into account mask in the ordering in grid_to_graph #18964

Merged
merged 12 commits into from Aug 30, 2021
Merged

FIX take into account mask in the ordering in grid_to_graph #18964

merged 12 commits into from Aug 30, 2021

Conversation

@bthirion
Copy link
Member

@bthirion bthirion commented Dec 3, 2020

Reference Issues/PRs

Fixes #18963

What does this implement/fix? Explain your changes.

In grid_to_graph , index the vertices according to mask structure, not their occurrence in edge matrix.

Any other comments?
@bthirion bthirion mentioned this pull request Dec 8, 2020
Closed
@glemaitre glemaitre self-requested a review Dec 15, 2020
glemaitre
glemaitre reviewed Dec 15, 2020
View changes
Copy link
Contributor

@glemaitre glemaitre left a comment

Apart from an additional test, LGTM

Please add an entry to the change log at doc/whats_new/v*.rst. Like the other entries there, please reference this pull request with :pr: and credit yourself (and other contributors if applicable) with :user:.

sklearn/feature_extraction/tests/test_image.py Show resolved Hide resolved
sklearn/feature_extraction/tests/test_image.py Outdated Show resolved Hide resolved
ateffal
ateffal reviewed Dec 16, 2020
View changes
Copy link

@ateffal ateffal left a comment

Effectively, edges.ravel() (line 80 ) does not contain isolated vertices (vertex of an edge, that was not eliminated by the mask ) while np.where(mask.ravel()) include all vertices.

This is the consequence of the logical and in line 71 : if one of the 2 vertices of an edge is eliminated by the mask, then both are eliminated in the edges array (after line 73).

@glemaitre glemaitre changed the title fixed grid_to_graph ording issue FIX take into account mask in the ordering in grid_to_graph Dec 16, 2020
@bthirion
Copy link
Member Author

@bthirion bthirion commented Dec 16, 2020

Please add an entry to the change log at doc/whats_new/v*.rst. Like the other entries there, please reference this pull request with :pr: and credit yourself (and other contributors if applicable) with :user:.

Shall I create a v1.0.rst ? that would be amazing !

@glemaitre
Copy link
Contributor

@glemaitre glemaitre commented Dec 16, 2020

Shall I create a v1.0.rst ? that would be amazing !

You don't need to create the file, only to merge master into your branch (you will just have to remove v1.00.rst.
And yep, 1.0 will be out in 6 months hopefully :)

@bthirion bthirion force-pushed the grid_to_graph_ordering branch from b369d0d to ba14027 Dec 16, 2020
glemaitre
glemaitre approved these changes Dec 16, 2020
View changes
Copy link
Contributor

@glemaitre glemaitre left a comment

LGTM (only a nitpick regarding the what's new entry).

doc/whats_new/v1.0.rst Show resolved Hide resolved
sklearn/feature_extraction/tests/test_image.py Show resolved Hide resolved
Base automatically changed from master to main Jan 22, 2021
@bthirion bthirion mentioned this pull request Feb 10, 2021
Open
@dPys
Copy link

@dPys dPys commented Feb 10, 2021

Another related improvement could be a thresholding parameter in the case of an img that zeros out connections for edge weights below a certain value?

@bthirion bthirion force-pushed the grid_to_graph_ordering branch from 1a41ef1 to fbd132d Feb 23, 2021
@bthirion
Copy link
Member Author

@bthirion bthirion commented Feb 24, 2021

Sorry, I don't understand the Changelog failure... Can anybody help ?

@cmarmo
Copy link
Member

@cmarmo cmarmo commented Feb 24, 2021

@bthirion, do you mind synchronizing with upstream? My typo in the workflow has been fixed in main. This should fix the failure. Sorry for that and thanks for your collaboration.

@bthirion bthirion force-pushed the grid_to_graph_ordering branch from c61b28c to f4ea8a0 Feb 24, 2021
@bthirion
Copy link
Member Author

@bthirion bthirion commented Feb 24, 2021

Working, thx !

@bthirion
Copy link
Member Author

@bthirion bthirion commented Apr 27, 2021

Any second approval ? ;-)

@glemaitre glemaitre added this to the 1.0 milestone Apr 27, 2021
TomDLT
TomDLT approved these changes Jun 8, 2021
View changes
sklearn/feature_extraction/image.py Outdated
order = np.searchsorted(np.unique(np.where(mask.ravel())),
np.arange(maxval + 1))
Copy link
Member

@TomDLT TomDLT Jun 8, 2021

Choose a reason for hiding this comment

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

Suggested change
order = np.searchsorted(np.unique(np.where(mask.ravel())),
np.arange(maxval + 1))
order = np.searchsorted(np.flatnonzero(mask), np.arange(maxval + 1))

More readable and probably faster.

@adrinjalali
Copy link
Member

@adrinjalali adrinjalali commented Aug 20, 2021

@bthirion mind accepting the last comment and merging with main?

@bthirion bthirion force-pushed the grid_to_graph_ordering branch from b679b62 to 5ada503 Aug 22, 2021
@bthirion
Copy link
Member Author

@bthirion bthirion commented Aug 22, 2021

Should be OK now. Best,

@glemaitre
Copy link
Contributor

@glemaitre glemaitre commented Aug 30, 2021

@adrinjalali do you want to have a look at this one now that the merge conflict is solved.

@glemaitre glemaitre merged commit 0cd6664 into scikit-learn:main Aug 30, 2021
29 checks passed
@glemaitre
Copy link
Contributor

@glemaitre glemaitre commented Aug 30, 2021

Actually, @TomDLT already reviewed as well. Merging then. Thanks @bthirion

samronsin added a commit to samronsin/scikit-learn that referenced this issue Nov 30, 2021
@bthirion @samronsin
FIX take into account mask in the ordering in grid_to_graph (scikit-l…
f51dd6f 
…earn#18964)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ateffal ateffal

@glemaitre glemaitre

@TomDLT TomDLT

Assignees
No one assigned
Labels
module:feature_extraction
Projects
None yet
Milestone
1.0
Linked issues

Successfully merging this pull request may close these issues.

sklearn.feature_extraction.grid_to_graph may reorder vertices
7 participants
@bthirion @glemaitre @dPys @cmarmo @adrinjalali @TomDLT @ateffal