Improve toBest when no candidates meet the cutoff
#347
Conversation
This patch changes the semantics of `toBest`'s cut-off number in the case where no candidate conversion meets the cut-off. The old semantics could be described as: > `toBest` finds the conversion whose scalar is closest to the cut-off, > without exceeding it. The new semantics can be described as: > `toBest` finds the conversion whose scalar is closest to the cut-off, > preferring to not exceed the cut-off if at all possible. Consider a system with the units `["mm", "cm", "m"]`. If the user asks for the "best" representation of 0.01 cm, candidate answers are 0.1 mm, 0.01 cm, or 0.0001 m. Of these, none meets the standard cutoff of having a scalar value at least 1. But we can still see intuitively that 0.1 mm is the answer that would be most readable to a human. Previously, this library at 3.0.0-beta.6 would return `null` in such a case, but at 3.0.0-beta.7 it returns the input value unchanged---even if it was in a different system of units than the one requested! Callers could work around the `null`-return behavior by doing extra logic in that case, but the change to just return the input made it possible to distinguish this "couldn't find an answer" case from one in which the input was legitimately the best result. I believe that this behavior is more natural than the existing behavior when using the default cut-off of 1, and in a GitHub-wide code search I could not find any public examples of users setting the value to something other than 1. This resolves convert-units#346 by making it so that we no longer need to call `toBest` with a cut-off value of 0. :-) Test Plan: All existing tests pass. Unit tests added, which fail before this commit and pass after it. wchargin-branch: tobest-soft-cutoff wchargin-source: 6b312ca8e98572c60ab2e5cae4d58724561a16af
There was a problem hiding this comment.
notes on prior behavior, now changed
| const convert = configureMeasurements<'length', LengthSystems, LengthUnits>({ | ||
| length, | ||
| }); | ||
| const actual = convert(0.5e-3).from('μm').toBest({ cutOffNumber: 1 }), |
There was a problem hiding this comment.
Prior to this patch, converting 0.0005 μm to best returned 0.0005 μm instead of 0.5 nm.
| const actual = convert(inputInches) | ||
| .from('in') | ||
| .toBest({ cutOffNumber: 1, system: 'metric' }), |
There was a problem hiding this comment.
Prior to this patch, asking for the best metric-system representation of 1.968e-8 inches gave 1.968e-8 inches, even though that is an imperial measurement!
|
Hello! This is a semantic change to We use As always, open to your feedback/thoughts. |
|
Sorry for the late reply and thank you for taking the time to look into improving the |
| const convert = configureMeasurements<'length', LengthSystems, LengthUnits>({ | ||
| length, | ||
| }); | ||
| const inputInches = convert(0.5).from('nm').to('in'); |
There was a problem hiding this comment.
I would prefer the value that is going to be used in the test be in-lined rather than using the library to calculate that value. I believe it would make the test little brittle and remove the possibility of the test failing if there is a regression in the to method which would add a fair bit of noise. In general, I like the tests to be as lean as possible and only utilize code that is absolutely necessary for the test. I also believe that it makes the test more readable given that I wouldn't need to convert 0.5nm to in in order to see what the input value is.
| const meetsCutOff = (result: BestResult<TUnits>): boolean => | ||
| isNegative ? result.val <= cutOffNumber : result.val >= cutOffNumber; | ||
|
|
||
| // Any result that meets the cutoff is better than any result that doesn't. | ||
| // Among candidates that meet the cutoff, smaller results (closer to the | ||
| // cutoff) are better. Among candidates that fail the cutoff, larger | ||
| // results (again, closer to the cutoff) are better. | ||
| const isBetter = ( | ||
| candidate: BestResult<TUnits>, | ||
| best: BestResult<TUnits> | ||
| ): boolean => { | ||
| const candidateMeetsCutOff = meetsCutOff(candidate); | ||
| const bestMeetsCutOff = meetsCutOff(best); | ||
| if (candidateMeetsCutOff && !bestMeetsCutOff) { | ||
| return true; | ||
| } | ||
| if (bestMeetsCutOff && !candidateMeetsCutOff) { | ||
| return false; | ||
| } | ||
| if (candidateMeetsCutOff) { | ||
| return isNegative ? candidate.val > best.val : candidate.val < best.val; | ||
| } else { | ||
| return isNegative ? candidate.val < best.val : candidate.val > best.val; | ||
| } | ||
| }; |
There was a problem hiding this comment.
I really like how clean and readable this code is! It's a large improvement over the original. However, I am not a fan of defining a function within another function unless it is being returned (a closure). I would like to see these functions placed inline or turned into private class methods. Though, I do think that the meetsCutOff could be inline as the variable names are descriptive enough. I really don't see the benefit of having a function for a single line of logic that is used in two places which are right beside one another.
| candidate: BestResult<TUnits>, | ||
| best: BestResult<TUnits> |
There was a problem hiding this comment.
Since the .val property is the only property being accessed, this function should accept number instead of BestResult<TUnits>. However, I'm wondering if this function definition could be improved to make it a bit more explicit which of the two arguments the returning boolean value is actually referring too. It's sort of implied but when I just look at the function being using, isBetter(candidate, best), it's not super clear to me. The variable names sort of hint at it but I still need to read the function's body to find out if my assumption is correct.
Which makes me wonder if the function signature was this: (candidate: BestResult<TUnits>, best: BestResult<TUnits>): BestResult<TUnits>, it would be a lot more clear and easier to reason about at a glance. Maybe the null check could be handled by the function as well: (candidate: BestResult<TUnits>, best: BestResult<TUnits> | null): BestResult<TUnits>. Returning the candidate if best is null.
This would also, in my opinion, simplify the code at line 282 to 289.
best = isBetter(
{
val: this.to(possibility),
unit: possibility,
singular: unit.singular,
plural: unit.plural,
},
best,
);I'm not sure if the name isBetter since makes sense in this case but at the moment I am not able to think of a better name. Maybe you have some ideas?
The function would also need to know if the original value is negative and also the cut off number. So maybe a closure would make sense? With that approach it would be possible to return a different function based on isNegative since it does not change once it is set. Not sure if removing a simple check would make a huge difference. Though, there is a possibility that doing so could also make the code harder to read. Maybe some experimentation is warranted :)
Anyways, sorry that was a bit rambly. What are your thoughts about all of this? Am I off base here?
This patch changes the semantics of
toBest's cut-off number in the case where no candidate conversion meets the cut-off. The old semantics could be described as:The new semantics can be described as:
Consider a system with the units
["mm", "cm", "m"]. If the user asks for the "best" representation of 0.01 cm, candidate answers are 0.1 mm, 0.01 cm, or 0.0001 m. Of these, none meets the standard cutoff of having a scalar value at least 1. But we can still see intuitively that 0.1 mm is the answer that would be most readable to a human.Previously, this library at 3.0.0-beta.6 would return
nullin such a case, but at 3.0.0-beta.7 it returns the input value unchanged---even if it was in a different system of units than the one requested! Callers could work around thenull-return behavior by doing extra logic in that case, but the change to just return the input made it possible to distinguish this "couldn't find an answer" case from one in which the input was legitimately the best result.I believe that this behavior is more natural than the existing behavior when using the default cut-off of 1, and in a GitHub-wide code search I could not find any public examples of users setting the value to something other than 1.
This resolves #346 by making it so that we no longer need to call
toBestwith a cut-off value of 0. :-)Test Plan:
All existing tests pass. Unit tests added, which fail before this commit and pass after it.
wchargin-branch: tobest-soft-cutoff