ndigits: make base and pad keywords #27908
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Marking this for triage. |
|
Agree; this seems like a pretty obvious place where we should be consistent. |
|
Triage approves. Restarting Travis; merge when it passes. |
JeffBezanson
force-pushed
the
rf/ndigits-base-kw
branch
from
e8be24d to
f56e3ec
Compare
|
The deprecation needed some tweaking, and I also happened to find some other missing deprecations at the same time. |
rfourquet
force-pushed
the
rf/ndigits-base-kw
branch
from
f56e3ec to
1791d76
Compare
Nice thanks. In turn your new deprecations needed some tweaking :) Maybe this time will be good |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
My initial motivation was that
ndigits(x, [base])andlog([base], x)do quite related things (ndigits(x, base)andlog(base, x)are numerically close), but they take their arguments in reverse order, which is rather confusing. One fix would be to reverse the arguments ofndigits, but asndigitsis also closely related todigits, it seems nicer to use keywords, to follow thedigitsAPI. Also, this allows to specifypadwhile using the default value forbase-- which also removes one TODO from the code :)I had started to do the same for
Base.ndigits0z, but I noticed some performance degradation, so I suspect that constant propagation does not happen with keyword argument... Asndigits0zmay be used in somewhat more performance sensitive code and is not exported, I propose to keep its old API for now (and usingpad=0inndigitshas the same behavior asndigits0z, sondigitsis enough for user-facing API).