-
Notifications
You must be signed in to change notification settings - Fork 73
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
Handle IEEE Floating Point Special Values #105
Conversation
Question: why are we not having the check be if v*y != 0 etc? |
The zero handling affects the space complexity of reverse mode. Shouldn’t we keep that and instead have the code do both the product v y != 0 and the -1 trick? |
The CI is stuck due to the GitHub Actions setup using an unsupported Ubuntu version. I've pushed a commit to |
Done, thank you! I am confused by what is going on with the CI on ghc-9.0.2 though. I looks like it cannot infer the
We are. It used to be
I am not sure what you mean by this. The space complexity should be unchanged, and I have not seen any indication of it being different. The tape is still the same (except for containing |
It looks like the failure on ghc-9.0.2 is related to the simplified subsumption change and can be worked around via eta-expansion. Then ghc-9.2.4 introduces the |
{ | ||
buffer[i] += v*x; | ||
double x = v * pTape->val[idx*2]; | ||
if (x != 0) buffer[i] += x; |
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.
oh i see what you mean, i didn't realize this new x was the old x * v stuff
i was just not reading the code correctly, thanks for clarifying |
Alright, it has been a few weeks, what is the verdict on this? Is more feedback needed? Is more work needed on my part? It would be nice if some people using the |
Sorry for forgetting about this—this dropped off my radar. A handful of observations:
|
I think this change will not severely impact performance, since branches shouldn't be the bottleneck |
otoh, i could be missing something |
Sorry for the delay, the last few weeks have been very busy for me.
I am not 100% sure, but I believe that this is due to the function in my application having more independent parts. Although the Jacobian in my application does not contain any zeroes (meaning that each parameter can influence every entry in the output vector), there are still subexpressions that arise as part of the calculation that only influence a subset of the entries in the output vector. Thus, the partial derivatives of some output entries with respect to these subexpressions could be zero. Since these zero entries are no longer skipped entirely, the performance is impacted. My guess is that this occurs to a lesser degree in the It might be a good idea to add a more decomposable benchmark, but I currently do not have the time to do this.
It could be a good idea, but I will leave that up to you, since it is mainly a matter of library design and maintenance burden considerations. |
Thanks, that information is helpful to know. Since we don't really have a good way to gauge if there are any other users of this That being said, it would be nice to track this fast-math idea in an issue so that we can revisit it later if someone comes along later and asks for a more performant
I'd be happy to merge this afterwards. Thanks! |
Done and done. Once this is merged, I can also reference the relevant code location from #106 so that it is easy to find the starting point for working on this issue. |
Thanks, @julmb! Let me know if you'd like a new |
Thank you, and I'm not in any rush, no worries. |
Alright, it's been a while, and I could actually use a new release with these changes as my test suite fails without them. Would it be possible to do a release on Hackage? |
Sorry for the delay on this! I've uploaded |
No worries, and thank you very much! |
This PR adds several changes primarily aimed at addressing issue #104.
Expand Test Suite
I added tests for the combinators
diff
,grad
,jacobian
, andhessian
. These include both basic correctness tests as well as specific regression tests for issue #104. Some of the tests were taken directly from the package documentation. For instance, the moduleNumeric.AD.Mode.Reverse
lists several examples for its combinators.The test suite is currently instantiated for the modes
Numeric.AD.Mode.Reverse
andNumeric.AD.Mode.Reverse.Double
but could easily be extended to cover the other modes. However, there are failing tests in some of the other modes and I am not sure if adjusting those should be part of this pull request. I am also not sure if/when I will have time to look at these, so the test suite is restricted toNumeric.AD.Mode.Reverse
andNumeric.AD.Mode.Reverse.Double
for now.It is worth noting that with the current setup, it is not possible to test both the pure and the
+ffi
version ofNumeric.AD.Mode.Reverse.Double
at the same time, since they are implemented within the same module and activated via preprocessor switch. This is especially troublesome since the trickier+ffi
version is not active by default. It would be nice if the+ffi
version could be its own module, but I do not know the rationale behind the original preprocessor switch design.Adjust
+ffi
Version ofNumeric.AD.Mode.Reverse.Double
There are three changes.
ReverseDouble
with+ffi
produces different result for square root of product #104, the current implementation uses index0
and value0.0
for the unused second operand in tape entries corresponding to unary operations. I changed this to use index-1
so that these unused operands can be distinguished from actual operands with index0
and value0.0
. This is a pure refactoring and should not change any behavior.0.0
in order to decide whether to skip this operand for backpropagation, it now checks if the index is>= 0
. This allows processing tape entries properly in the presence ofNaN
andInfinity
values and makes the teststests.reverse-double.issue-104.inside
pass. However, it no longer skips over entries where the backpropagation really would not have an effect on the partial derivative.0.0
, it now checks if the increments to each of the partial derivatives are0.0
. This allows correct processing ofNaN
andInfinity
values and makes the tests undertests.reverse-double.issue-104.outside
pass. It also restores some of the skipping lost in change 2. However, there is still a performance impact compared to the original behavior of skipping the entire update if the corresponding partial derivative is0.0
.For the
bench/BlackScholes.hs
benchmark (adjusted forNumeric.AD.Mode.Reverse.Double
), I was not able to measure any performance impact of these changes. For two of my own applications, I measured an increase in runtime by a factor of 1.07 and 1.21 respectively.