Skip to content
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

Todo list discussion #25

Closed
goedman opened this issue Nov 21, 2018 · 18 comments
Closed

Todo list discussion #25

goedman opened this issue Nov 21, 2018 · 18 comments

Comments

@goedman
Copy link
Member

goedman commented Nov 21, 2018

Items on the list:

  1. Add documentation (inline and github.io)
  2. Discussion about single list of first and second derivatives
  3. Functions dropped in the v4.0.0 release (with reason why?)
  4. Any functionality lost? (I don't think so)
  5. Discussion about single list of first and second derivatives
    5.1 Split into 2 lists?
    5.2 If accepted remove from HyperDualNumbers
  6. More tests (Sage? Symata?)
  7. Have all '==' tests been replaced by isequal()
  8. Benchmark old vs. new version
  9. Improve coverage
  10. Layout README needs attention
@briochemc
Copy link
Contributor

briochemc commented Nov 26, 2018

Hi Rob,

About the ==, and isequal overloads. After a few discussions on slack, I now have a slightly better general view on how Julia packages treat dual numbers. ForwardDiff.jl choses that Dual{Real} <: Real
(with ForwardDiff.Dual(0, 1) == 0 and isequal(ForwardDiff.Dual(0, 1), 0) while HyperDualNumbers (and DualNumbers) have Dual{Real} <: Number (with Dual(0, 1) == 0 but not isequal(Dual(0, 1), 0). I think the DualNumbers and ForwardDiff packages are set on going for the ForwardDiff way, i.e., set Dual{Real} <: Real and have both ε == 0 and isequal(ε, 0).

I think it could be a good idea to "test" having ε₁, ε₂, and ε₁ε₂ not equal to 0 in HyperDualNumbers (this is what I sort of did with the overload of abs so that abs(-x^2) is correct for example). Having ε₂ > 0 would mean that things like ε₂ * sparse([1, 2], [1, 2], ones(2)) does not evaluate to an empty sparse matrix (while this currently evaluates to the empty sparse matrix because ε₂ == 0 in the current version). However this would also mean that we do things a bit differently from ForwardDiff.jl (and from what DualNumbers is going to become eventually). What do you think?

@goedman
Copy link
Member Author

goedman commented Nov 26, 2018 via email

@briochemc
Copy link
Contributor

I’m not quite sure about your sentence below:

This would mean that things like ε₂ * sparse([1, 2], [1, 2], ones(2)) would not result an empty sparse matrix because ε₂ > 0

Can I rephrase that as “This means that …” in the current implementation. Or is that currently only the case for abs?

What I mean is that opting for ε > 0 instead of ε == 0 would change the behavior of duals with sparse matrices. (I use ε here for simplicity but I mean it for any of ε₁, ε₂ or ε₁ε₂.) This is because ε == 0 implies ε * A is empty for any sparse matrix A: In other words, when multiplying by ε you risk to inadvertently cancel some terms. See below how all of the ε₁ terms do not make it into ε₁ * spA:

julia> A = Matrix(1.0I, 2, 2)
2×2 Array{Float64,2}:
 1.0  0.0
 0.0  1.0

julia> ε₁ * A # multiplying A by ε₁ works as expected
2×2 Array{Hyper{Float64},2}:
 0.0+1.0ε₁+0.0ε₂+0.0ε₁ε₂  0.0+0.0ε₁+0.0ε₂+0.0ε₁ε₂
 0.0+0.0ε₁+0.0ε₂+0.0ε₁ε₂  0.0+1.0ε₁+0.0ε₂+0.0ε₁ε₂

julia> spA = sparse(A) # try the same thing with sparse matrix
2×2 SparseMatrixCSC{Float64,Int64} with 2 stored entries:
  [1, 1]  =  1.0
  [2, 2]  =  1.0

julia> ε₁ * spA # multiplying spA by ε₁ gives an empty sparse matrix
2×2 SparseMatrixCSC{Hyper{Float64},Int64} with 0 stored entries

For abs: In the current implementation, HyperDualNumbers uses abs(0 - ε₁) == 0 + 1ε₁ + 0ε₂ + 0ε₁ε₂ as if ε₁ was positive and reflected (while DualNumbers uses abs(0 - ε) == -ε). This is why I said that abs is implemented "as if" ε₁ > 0.

What works best for your publication?

Anything works :) None of these implementation details are really relevant for the type of paper I am writing — I just want to properly acknowledge the packages that I use!

Are they planning to change DualNumbers?

I don't know. I think they might make isequal(ε, 0), like ForwardDiff currently does (not sure this qualifies as a "change" really though). However, I am pretty sure they are fixed on ε == 0 (i.e., will not change to ε > 0).

I do think we should be consistent.

This is what bothers me most. I don't think there is any rush, but I thought I should lay out the suggestion sooner rather than later. It mostly has to do with how HyperDualNumbers will "percolate" through generic code code with if x == 0 or if x > 0 lines.

I'm currently in Irvine but I should go (back) to Sydney next year :)

@goedman
Copy link
Member Author

goedman commented Nov 26, 2018

Thank you, get it now.

I would be ok to publish the current version with this consideration documented.

@goedman
Copy link
Member Author

goedman commented Nov 28, 2018

Hi Benoit, shall we release version 4.0.0?

@briochemc
Copy link
Contributor

Hi Rob,

OK! Done!

@goedman
Copy link
Member Author

goedman commented Nov 28, 2018

Great, thanks! I noted NaNMath was missing from REQUIRE so added it and re-released. Let's see if that fixes the JuliaCIBot issue.

@goedman
Copy link
Member Author

goedman commented Nov 29, 2018

Oops, and Calculus of course!

@goedman
Copy link
Member Author

goedman commented Nov 29, 2018

Hi Benoit. not completely sure what is happening but it seems to me the JuliaCIBot has evolved now to the point where it checks packages that depend on the package being registered. This creates a problem as both packages export \varepsilon_1 etc. It needs

It does require qualification. Reexport might not work I think because the epsilon discussions are not equal (I think). Maybe using a different symbol?

Interesting package!

@briochemc
Copy link
Contributor

Hi Rob,

I think something wnet wrong in the formatting of your last post... I'm not sure I understood the problem. What other packages export ε₁, ε₂, or ε₁ε₂? I'll have a look at the JuliaCIBot failure now.

@briochemc
Copy link
Contributor

Oh I did not know it would test my matrix packages! OK I will remove those from it :)

@briochemc
Copy link
Contributor

I'm not this will work but we'll see :)

@briochemc
Copy link
Contributor

Interesting package!

Thanks!

@goedman
Copy link
Member Author

goedman commented Nov 29, 2018

Sorry for not mentioning which package!

What do you mean by "I will remove those from it"? My assumption is (but I could be way off) that JuliaCIBot creates a list of up to 10 packages which rely on the a package and tests if these still work. What surprises me is that in that case we're updating the major revision number which indicates the changes can break things.

@briochemc
Copy link
Contributor

briochemc commented Nov 29, 2018

What do you mean by "I will remove those from it"?

I removed the definitions of ε₁, ε₂, and ε₁ε₂ and their corresponding tests in HyperDualMatrixTools.jl. Let's see if this works :)

We might have to force a second run of the JuliaCIBot by deleting the release and making a new one. (But I guess you have been doing that already for adding the package dependencies in REQUIRE?)

@goedman
Copy link
Member Author

goedman commented Nov 29, 2018

Hi Benoit, yes, I'll wait with creating a new release for HyperDualMatrixTools.jl to pass.

Not sure why the 'test fail' output points to line 14 in runtests.jl of HDMT while on my system it fails on line 22 (inside factorize()).

@briochemc
Copy link
Contributor

Hi Rob,

I asked around on slack and it turns out in cases like this it has to be manually accepted: So I wrote a comment saying that breaking HyperDualMatrixTools is intentional and I will release a new version of it once HyperDualNumbers gets registered.

@goedman
Copy link
Member Author

goedman commented Nov 30, 2018

Thanks Benoit, seems we're done!

@goedman goedman closed this as completed Feb 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants