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 min_magnitude & max_magnitude passthrough in complex-valued from_dtype() #3570

Merged
merged 10 commits into from Feb 4, 2023

Conversation

felixdivo
Copy link
Contributor

Closes #3468.

@felixdivo felixdivo marked this pull request as ready for review February 4, 2023 11:07
@felixdivo
Copy link
Contributor Author

There is again a problem with RELEASE.rst. 🙃

Copy link
Member

@honno honno left a comment

Choose a reason for hiding this comment

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

Really appreciate you working on these issues :)

Things are looking good! I'll have a proper look later on if Zac doesn't get round to it heh.

Also ICYMI, you can use ./build.sh format at the root of the repository to run the linters locally.

hypothesis-python/src/hypothesis/extra/numpy.py Outdated Show resolved Hide resolved
Copy link
Member

@Zac-HD Zac-HD left a comment

Choose a reason for hiding this comment

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

A few comments left, but this looks great - thanks again Felix!

hypothesis-python/src/hypothesis/extra/numpy.py Outdated Show resolved Hide resolved
@Zac-HD
Copy link
Member

Zac-HD commented Feb 4, 2023

OK, looks like there's one remaining test failure due to this new passthrough logic for width.

  • here, adding , ids=repr to the parametrize call will let us tell which dtype caused the problem
  • I think then we want to limit the width argument to the underlying strategy as we do for floats here, so that we can e.g. generate complex128 elements and store them in a complex256 array.

@Zac-HD Zac-HD enabled auto-merge February 4, 2023 19:25
@felixdivo
Copy link
Contributor Author

There is again a problem with RELEASE.rst. 🙃

This is still the case.

Should I change anything else?

@Zac-HD
Copy link
Member

Zac-HD commented Feb 4, 2023

Ah, you'll need to create that file with your changelog entry for this PR. I'd suggest something like:

RELEASE_TYPE: minor

This release allows for more precise generation of complex numbers using
:func:`~hypothesis.extra.numpy.from_dtype`, by supporting the ``width``,
``min_magnitude``, and ``min_magnitude`` arguments (:issue:`3468`).

Thanks to Felix Divo for this feature!

The underlying cause: it looks like you branched off from a point where we'd merged a previous PR to master, but not yet finished deploying that release. This is a known issue but we can't easily fix it until merge queues are available 😥 - until then we'll just have to deal with occasionally rebasing or fixing merge conflicts by hand.

@Zac-HD Zac-HD disabled auto-merge February 4, 2023 19:31
@felixdivo
Copy link
Contributor Author

Ah, you'll need to create that file with your changelog entry for this PR.

@Zac-HD I now replaced the currently present file. I hope that was correct. I don't have sufficient access rights to resolve the conflict though ^^.

And thanks for the kind note. 😊

@Zac-HD
Copy link
Member

Zac-HD commented Feb 4, 2023

Not quite - you'll need to git switch master; git pull upstream master ; git switch - ; git rebase master (or git merge master, but that's uglier) and either way then fix the merge conflict 🙂

@felixdivo
Copy link
Contributor Author

felixdivo commented Feb 4, 2023

Resolved @Zac-HD :)

Copy link
Member

@Zac-HD Zac-HD left a comment

Choose a reason for hiding this comment

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

🎉 let's ship it!

@Zac-HD Zac-HD merged commit 10b61a5 into HypothesisWorks:master Feb 4, 2023
@felixdivo felixdivo deleted the from_dtype-complex-bounds branch February 5, 2023 09:06
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.

Add min/max magnitude arguments to from_dtype() strategy
3 participants