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

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

Merged
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?

Copy link
Member

@glemaitre glemaitre left a comment

Choose a reason for hiding this comment

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

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 Outdated Show resolved Hide resolved
Copy link

@ateffal ateffal left a comment

Choose a reason for hiding this comment

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

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

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
Member

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 :)

Copy link
Member

@glemaitre glemaitre left a comment

Choose a reason for hiding this comment

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

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

doc/whats_new/v1.0.rst Show resolved Hide resolved
@dPys
Copy link

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
Copy link
Member Author

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

@cmarmo
Copy link
Member

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
Copy link
Member Author

Working, thx !

@bthirion
Copy link
Member Author

Any second approval ? ;-)

@glemaitre glemaitre added this to the 1.0 milestone Apr 27, 2021
Comment on lines 80 to 81
order = np.searchsorted(np.unique(np.where(mask.ravel())),
np.arange(maxval + 1))
Copy link
Member

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

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

@bthirion
Copy link
Member Author

Should be OK now. Best,

@glemaitre
Copy link
Member

@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
@glemaitre
Copy link
Member

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

samronsin pushed a commit to samronsin/scikit-learn that referenced this pull request Nov 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

sklearn.feature_extraction.grid_to_graph may reorder vertices
7 participants