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 compat annotation for NaN handling in (l|r)mul! #30361

Merged
merged 4 commits into from
Dec 12, 2018

Conversation

tkf
Copy link
Member

@tkf tkf commented Dec 12, 2018

As discussed in #28972, the behavior of lmul! and rmul! was changed between 1.0 and 1.1. I think issue #28972 should be treated as an API documentation bug (and I think it would be nice if similar issues are closed by PRs addressing the issue). This PR adds docstring to elaborate the API of those functions and adds compat annotation for the incompatible change.

This patch also adds tests to check the documented behavior.
@tkf
Copy link
Member Author

tkf commented Dec 12, 2018

I just noticed #29726 has more discussion and it was considered actually a bug fix #29726 (comment)

Nevertheless, it's good to have docs and tests.

modified (unless the type of `b` defines a non-standard behavior).

!!! compat "Julia 1.1"
Prior to Julia 1.1, how `NaN` entries in `A` were treated was
Copy link
Member

Choose a reason for hiding this comment

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

Maybe rephrase to something like Prior to Julia 1.1, NaNentries inA were treated inconsistently.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, that's much smoother. I changed the docstring. I also mention Inf since that's also changed.

@fredrikekre fredrikekre added domain:docs This change adds or pertains to documentation backport pending 1.1 labels Dec 12, 2018
@fredrikekre fredrikekre merged commit 797ddbb into JuliaLang:master Dec 12, 2018
KristofferC pushed a commit that referenced this pull request Dec 12, 2018
@KristofferC KristofferC mentioned this pull request Dec 12, 2018
52 tasks
@tkf tkf deleted the lmul-nan branch December 13, 2018 00:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
domain:docs This change adds or pertains to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants