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

Renew the function subset_matches() in Class Subsetter #51

Merged
merged 4 commits into from
Feb 14, 2023

Conversation

algopert
Copy link
Contributor

@algopert algopert commented Feb 14, 2023

closes #50

@OpenStitching OpenStitching deleted a comment from algopert Feb 14, 2023
@lukasalexanderweber
Copy link
Member

I really like your approach of getting rid of get_indices_to_delete(), subset_matrix(), delete_index_from_matrix() .

I updated the subset_matches and tests to make it more compact. Please check if it works for your issue and then I think we are ready to merge :)

@algopert
Copy link
Contributor Author

Hi Lukas.

It works well.
I think there was error in either of 3 functions deleted.

By the way, I have a question.

There will be still the values smaller than threshold in matrix matches_subset.

image

By my simulation, it doesn't seem to affect the results.
Anyway, I will open another issue for that.

Let's merge.

Thanks.

Copy link
Contributor Author

@algopert algopert left a comment

Choose a reason for hiding this comment

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

I think np.ix_() is good approach.

And I don't have a PYTHONic mindset, just trying to reduce computational complexity as an algo developer.

:)

Copy link
Contributor Author

@algopert algopert left a comment

Choose a reason for hiding this comment

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

Very compact solution!

@lukasalexanderweber lukasalexanderweber merged commit 140004f into OpenStitching:main Feb 14, 2023
@lukasalexanderweber
Copy link
Member

Thanks a lot for your contribution!

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

Successfully merging this pull request may close these issues.

Bug in subsetter.subset_matches()
2 participants