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

doc: show that f32::log and f64::log are not correctly rounded #47277

Merged
merged 2 commits into from
Jan 15, 2018

Conversation

tspiteri
Copy link
Contributor

@tspiteri tspiteri commented Jan 8, 2018

Fixes #47273.

One thing I'm not sure about is whether the "calculated as self.ln() / base.ln()" bit is being too specific, maybe we do not want to make this such a strong commitment. I think it's fine, but we should not make commitments in the API documentation by accident.

In case that is removed, the added sentence "self.log2() can ... base 10." still makes it amply clear that the log methods can be more inaccurate than other methods. If the above clause is removed, this second sentence can be moved to the first paragraph, kind of like the accuracy comment for the mul_add method.

@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @aidanhs (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@hanna-kruppe
Copy link
Contributor

FWIW I'm not a big fan of putting the implementation in the docs.

@kennytm kennytm added the A-docs Area: documentation for any part of the project, including the compiler, standard library, and tools label Jan 8, 2018
@aidanhs
Copy link
Member

aidanhs commented Jan 9, 2018

I'm inclined to agree. What about removing the implementation details and saying something like

[...existing prose...] arbitrary base.

The result of this may not be correctly rounded due to implementation details - self.log2() can [...existing prose...]

?

@tspiteri
Copy link
Contributor Author

tspiteri commented Jan 9, 2018

I've removed the implementation details from the doc. (BTW, rust-highfive suggests extra commits for changes, which is what I did; if I should squash the commits into one please let me know.)

@hanna-kruppe
Copy link
Contributor

Adding new commits can be useful for big changes, but for small things like this I personally don't see the need. In any case, we prefer to squash before merging.

@carols10cents carols10cents added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 9, 2018
@aidanhs
Copy link
Member

aidanhs commented Jan 9, 2018

This seems good to me but I'd like to get a docs team member to take a look - r? @frewsxcv

@rust-highfive rust-highfive assigned frewsxcv and unassigned aidanhs Jan 9, 2018
@frewsxcv
Copy link
Member

looks good to me, thanks!

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Jan 13, 2018

📌 Commit 6d82e78 has been approved by frewsxcv

GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Jan 14, 2018
doc: show that `f32::log` and `f64::log` are not correctly rounded

Fixes rust-lang#47273.

One thing I'm not sure about is whether the "calculated as `self.ln() / base.ln()`" bit is being too specific, maybe we do not want to make this such a strong commitment. I think it's fine, but we should not make commitments in the API documentation by accident.

In case that is removed, the added sentence "`self.log2()` can ... base 10." still makes it amply clear that the `log` methods can be more inaccurate than other methods. If the above clause is removed, this second sentence can be moved to the first paragraph, kind of like the accuracy comment for the [`mul_add`](https://doc.rust-lang.org/std/primitive.f32.html#method.mul_add) method.
kennytm added a commit to kennytm/rust that referenced this pull request Jan 15, 2018
doc: show that `f32::log` and `f64::log` are not correctly rounded

Fixes rust-lang#47273.

One thing I'm not sure about is whether the "calculated as `self.ln() / base.ln()`" bit is being too specific, maybe we do not want to make this such a strong commitment. I think it's fine, but we should not make commitments in the API documentation by accident.

In case that is removed, the added sentence "`self.log2()` can ... base 10." still makes it amply clear that the `log` methods can be more inaccurate than other methods. If the above clause is removed, this second sentence can be moved to the first paragraph, kind of like the accuracy comment for the [`mul_add`](https://doc.rust-lang.org/std/primitive.f32.html#method.mul_add) method.
bors added a commit that referenced this pull request Jan 15, 2018
Rollup of 10 pull requests

- Successful merges: #47120, #47126, #47277, #47330, #47368, #47372, #47414, #47417, #47432, #47443
- Failed merges: #47334
@bors
Copy link
Contributor

bors commented Jan 15, 2018

☔ The latest upstream changes (presumably #47445) made this pull request unmergeable. Please resolve the merge conflicts.

@bors bors merged commit 6d82e78 into rust-lang:master Jan 15, 2018
@tspiteri tspiteri deleted the log-correctness branch January 15, 2018 15:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-docs Area: documentation for any part of the project, including the compiler, standard library, and tools S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants