Read abs and signum at zero, where both compositions are 0 (#892) - #899
Merged
Conversation
|sgn(z)| and sgn(|z|) are 1 for every z except 0, where both are 0 -- sgn(0) is 0, which the comment directly above one of these rules already said. Both answered 1 for any argument, so a symbolic one gave a value that is wrong at one point: abs(sgn(x)) and sgn(abs(x)) simplified to 1. The rules read the argument's DomainCondition, which is a different question from the one they needed. A bare x is defined everywhere and can itself be zero; x/x is defined only away from zero and is nonzero throughout. Reading the domain conflates the two. They now decide where the argument's value can be read and leave the node alone otherwise. A condition would have been the wrong fix, as in #884 and #887: the expression is defined at zero and equal to 0 there, so `1 provided not z = 0` would trade a wrong value for a wrong domain, and there is no closed form for "1 away from zero, 0 at it" other than the expressions themselves. The four x/x cases go with them, and that is a coverage loss rather than a correction: 1 provided not x = 0 was right for those. Telling them apart needs "nonzero throughout its domain", and the two tests available here cannot -- Evaled leaves x/x as x/x, and DomainCondition is what conflated them. Simplify would answer it and must not be called from inside InnerSimplify (#403). Written down in the test file rather than left as a silent regression. This is the third rule block in two days found to have a sound half and an unsound half written as though they were symmetric, after #884 and #887. Measured: 6333 C# tests pass, 130 F# tests pass, casbench 117/119 with 0 wrong, propcheck 0 failures, rootcheck 596/596, simpsweep 10463/10463 agreeing, and boundcheck 7 disagreements down to 4 -- the two this fixes, plus the two the merged #888 and #893 fixed.
The previous commit swept in a 1281-line BenchmarkDotNet log, because /Tests/DotnetBenchmark/benchmark_results.csv is anchored at the repository root and the directory is Sources/Tests/DotnetBenchmark/benchmark_results.csv. The entry has been inert since f0db3ee moved everything under Sources/, so the benchmark's output has been showing up in git status -- and in git add -A -- ever since. Five neighbours had the same problem and are corrected with it. The Sources/Samples entries a few lines below were already updated, which is why the file looked fine at a glance. Third path in this repository found pointing at the pre-f0db3eef layout, after amsite.fsx's publish path and NaiveStaticGenerator's read of AngouriMath.xml. Worth grepping for others.
2.0.0 has no column in this file: it is commit 1691 and the table stopped at the 1671st. The Math OS plan (#746) lists speed on popular use cases as a standing condition on every tier rather than a roadmap item, and the release met the correctness half of that exhaustively and the performance half not at all. Filling the gap with one new column would have been worse than leaving it. Measured here, every one of the sixteen rows came out at 0.5-0.6x of the 1671st -- a uniform factor, which is a faster machine and not faster code, and a reader comparing the columns would have seen a 1.75x improvement that does not exist. That is the mistake this file's own header warns about, so the 1671st was re-measured on the same machine minutes later and both are recorded as a pair that may be read as a ratio. The answer: no regression. Most rows move 1-5%, which by this file's standard says nothing. SimplifyEasy is +8.7%, which is above it, and the cause is likely ours rather than mysterious -- the corrections merged since the 1671st added guards that call Evaled inside a pattern's when clause on a path that used to match structurally. Recorded as a row to watch, with a note not to write more guards that way without measuring. Also corrects the previous commit here. Of the six .gitignore entries it repointed at Sources/, four name directories that no longer exist anywhere in the tree, so it made dead configuration look current. Those are deleted along with three more of the same kind; what remains are the two that match something real -- the benchmark's output directory, which is what started this, and the local ANTLR download.
Collaborator
Author
|
This conflicts with #901 ( Whichever merges second, the resolution is: keep both, and put this PR's One measurement from that work belongs here too, since it is about this PR's issue rather than #901's: |
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Closes #892. The fifth wrong answer of this class, and the third rule block found with a sound half and an unsound half written as though they were symmetric — after #884 and #887.
What was wrong
|sgn(z)|andsgn(|z|)are1for everyzexcept0, where both are0becausesgn(0)is0— which the comment directly above one of the two rules already says. Both answered
1for anyargument at all.
The rules read the argument's
DomainCondition, which answers a different question. A barexisdefined everywhere and can itself be zero;
x / xis defined only away from zero and is nonzerothroughout. Reading the domain conflates "the argument cannot be zero" with "the argument's zero is
outside its domain".
The fix, and what it deliberately is not
They now decide where the argument's value can be read —
0if it vanishes,1if it does not — andleave the node as written otherwise.
Not a
Providedf. The expression is defined at zero and equal to0there, so1 provided not z = 0would replace a wrong value with a wrong domain. That is §3 ofDocs/Contributing/SimplificationContract.md, and the third time this week the same trap has come up.Not a closed form either.
abs(sgn(x))is the indicator ofx != 0, andsgn(abs(x))is thesame function, so rewriting either into the other is circular. Decide, or decline.
One coverage loss, stated rather than hidden
signum(abs(x/x))andabs(signum(x/x))were1 provided not x = 0, which is correct, and theyare now left as written. Separating that case from the
xcase needs "nonzero throughout its domain",and neither test available inside
InnerSimplifycan express it:Evaledleavesx / xasx / x,DomainConditionis what conflated them in the first place, andSimplifywould answer it but mustnot be called from there (#403). Recorded in the test file and in
BREAKING-CHANGES.mdrather thanleft as a silent regression.
Four test rows moved for that reason and two because they pinned the wrong answer; the numeric cases
(
abs(sgn(0))→0,sgn(abs(-3))→1) were already right and are unchanged.Measured on this branch, .NET 10
casbenchpropcheckrootchecksimpsweepboundcheckThe four remaining boundcheck findings are all filed: #884's logarithm half,
log(x, x)'s over-strongcondition on #890, and the known
ln(x) + ln(x+1)widening.Second commit: six stale
.gitignorepaths/Tests/DotnetBenchmark/benchmark_results.csvis anchored at the repository root while the directoryis
Sources/Tests/DotnetBenchmark/…, so it has been inert sincef0db3eefmoved everything underSources/— and the benchmark's output has been appearing ingit statusever since. It swept intomy first commit here, which is how I noticed; that commit adds the log and this one removes it, so the
squashed result contains neither.
Five neighbours had the same fault and are fixed with it. This is the third path in the repository
found still pointing at the pre-
f0db3eeflayout, afteramsite.fsx's publish path andNaiveStaticGenerator's read ofAngouriMath.xml(both fixed in the site repo). Worth a grep forothers.