-
Notifications
You must be signed in to change notification settings - Fork 187
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
optional log base for ticks functions #233
Conversation
@mbostock any feedback on this? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like it! Please also add to the documentation in README.md (in particular, what happens to "1, 2, 5" when the base is not 10).
Another thing that could be tested and documented is what it gives for base e. It surprised me that it switches from units to multiples of e (it makes sense of course, given that 1 is e^0, but I wonder if we have an example of how this can be useful).
https://observablehq.com/d/90d87c2c822df2bd
@Fil That's a really good point about the
|
@Fil Is it possible for me to update the |
@Fil any update on this? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought I had left a comment, but I now find it was waiting in one of many many open tabs… Sorry! For the documentation on Observable, you can fork the notebook and make a suggestion.
Co-authored-by: Philippe Rivière <[email protected]>
My concerns here:
Overall, I think I would prefer to keep the existing methods as-is and have base-specific variants of d3.ticks and d3.tickIncrement for base = 2 (binary) and base = e (“natural”). |
That's understandable, I can update the pr to have base-specific function variants. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mbostock Sorry for the delay, I finally got around to updating this PR.
I want to follow up on the concerns you mentioned...
return [i1, i2, inc]; | ||
} | ||
|
||
export default function ticks(start, stop, count) { | ||
export default function ticks(start, stop, count, log) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Too many positional arguments (start, stop, count, base). Overall, I think I would prefer to keep the existing methods as-is and have base-specific variants of d3.ticks and d3.tickIncrement for base = 2 (binary) and base = e (“natural”).
I'm happy to separate these added methods into separate methods. But I'd like to avoid duplication of code. Would you be ok with having the log
option on a base function then exporting a wrapped function for each log type with just start
, stop
and count
args?
So exporting ticks
, ticksNatural
and ticksBinary
, along with their respective tickIncrement
functions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Other implicit dependencies on base 10 (e.g., 2 and 5 are factors of 10, and 10 appears elsewhere in code)
Could you point me to the other parts of the code that would need to be addressed?
const logParams = { | ||
binary: [Math.log2, 2], | ||
natural: [(n) => Math.log1p(n - 1), Math.E], | ||
common: [Math.log10, 10], | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tiny loss of precision and performance using Math.log(base) etc. instead of Math.LN10
Does this approach work for you to use the respective precise Math
methods for each allowed log type?
binary: [Math.log2, 2], | ||
natural: [(n) => Math.log1p(n - 1), Math.E], | ||
common: [Math.log10, 10], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In practice will probably only be used for base = 2 and base = e
Limits the log types to just common
, natural
and binary
.
Adds optional
base
value toticks
,tickIncrement
andtickStep
functions. Using defaultbase
value of10
results in no functional changes to existing code.These changes enable rendering linear ticks for binary, natural and other log bases.
cc: @monfera
closes #232