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

Add other correct test results to the test checks for divisive #7263

Closed
wants to merge 1 commit into from

Conversation

dschult
Copy link
Member

@dschult dschult commented Jan 30, 2024

Some tests had more than opne possible answer, but only checked for the one given by this implementation. Testing on nx-parallel as mentioned in networkx/nx-parallel#36 .

This PR adds the other possible correct test results to these tests.

Copy link
Contributor

@rossbar rossbar left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @dschult . As mentioned over in #7251 - there may be even more valid "answers" for some of these test cases, but no need to cover absolutely every case in this PR.

BTW +1 for the assignment expressions, very nice! (maybe the first use of this language feature in the library? 🎉 )

@dschult
Copy link
Member Author

dschult commented Feb 1, 2024

Unfortunately, this approach (with the walrus) won't work for these cases because it relies on the first part of the partition to try to identify which answer is being considered. And of course, the first part of the partition doesn't determine uniquely which answer we should consider. The solution merged in #7251 is preferred for this reason.

we'll find a use for walrii (is that the plural?) soon enough. :) We just need to make sure it is as readable as a version that doesn't use it.

@dschult dschult closed this Feb 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

2 participants