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

Fixes #3185: strings broadcast bug #3210

Merged
merged 1 commit into from
May 21, 2024

Conversation

stress-tess
Copy link
Member

This PR fixes #3185 by removing the custom string permutation logic in favor of broadcasting the indices and then indexing into the segstring. I realized that we overcomplicating things and that optimizing this solution would essientailly be duplicating the segstring indexing code. I don't know what lead me to take this approach in the first place, but this is much cleaner and makes use of existing well-tested and optimized functions

Copy link
Contributor

@ajpotts ajpotts left a comment

Choose a reason for hiding this comment

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

Looks good! Thanks for fixing this.

This PR fixes Bears-R-Us#3185 by removing the custom string permutation logic in favor of broadcasting the indices and then indexing into the segstring. I realized that we overcomplicating things and that optimizing this solution would essientailly be duplicating the segstring indexing code. I don't know what lead me to take this approach in the first place, but this is much cleaner and makes use of existing well-tested and optimized functions
@stress-tess stress-tess force-pushed the 3185_str_broadcast_bug branch from 512c512 to ded4584 Compare May 21, 2024 19:43
@stress-tess stress-tess merged commit 01bfe3f into Bears-R-Us:master May 21, 2024
22 checks passed
@stress-tess stress-tess deleted the 3185_str_broadcast_bug branch May 21, 2024 20:07
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 strings broadcast
3 participants