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

Inconsistent behavior in sparse matrices when interacting with NaNs #18091

Closed
gabgoh opened this issue Aug 17, 2016 · 11 comments
Closed

Inconsistent behavior in sparse matrices when interacting with NaNs #18091

gabgoh opened this issue Aug 17, 2016 · 11 comments
Labels
sparse Sparse arrays

Comments

@gabgoh
Copy link

gabgoh commented Aug 17, 2016

julia> spzeros(2,2)*[NaN;NaN]    
2-element Array{Float64,1}:      
 0.0                             
 0.0                             

julia> zeros(2,2)*[NaN;NaN]      
2-element Array{Float64,1}:      
 NaN                             
 NaN            

I believe the latter behavior is the correct one

@kshyatt kshyatt added the sparse Sparse arrays label Aug 17, 2016
@tkelman
Copy link
Contributor

tkelman commented Aug 17, 2016

Yes, this is wrong, our current sparse matrix implementation does not give proper IEEE754 results for operations on the implicit non-stored zeros. Should be fixable with a bit of care.

related: #14973

@KristofferC
Copy link
Member

KristofferC commented Aug 17, 2016

Is there any programming language or library that does this properly for all operations?

@JaredCrean2
Copy link
Contributor

Oh, this again. There was also some discussion in #15579 revolving around handling Infs and NaNs in sparse algorithms.

@ViralBShah
Copy link
Member

Cc @Sacha0

@tkelman
Copy link
Contributor

tkelman commented Aug 18, 2016

I'll repeat myself here too:

Ignoring the corner cases doesn't make Julia's behavior materially more correct here. Fixing it, for the cases where that's not terribly complicated to do [...] means we're ahead of the curve in fixing bugs that have either not been reported yet, not been fixed yet, or been left unaddressed in existing libraries. We don't need to repeat the mistakes of our predecessors here.

Calling this a feature instead of a bug runs counter to the purpose of sparse matrices as a representation optimization that shouldn't change visible behavior in a generic programming sense.

@JaredCrean2
Copy link
Contributor

@tkelman I think you and I should get matching shirts and wear them to the next JuliaCon: https://www.amazon.com/Arguing-Engineer-Like-Wrestling-shirt/dp/B01HJ85TTC

Ok, if you are really intent on doing things this way, how about structuring the API in this way:

function func_unchecked(args...)
   # implement the algorithm, not performing any checks
end

function func_checked(args...)
  # do checks here
  func_unchecked(args...)
end

and export both versions, and also link this up to the @fastmath machinery so when the user decides they don't care about Infs and NaNs, calls to func_checked turn into func_unchecked?

@tkelman
Copy link
Contributor

tkelman commented Aug 19, 2016

that seems fine, except that I don't think we should export the unchecked version

@JaredCrean2
Copy link
Contributor

Why not? It just unchecked, not unsafe. The functions that do arithmetic don't check for overflow, underflow etc.

@tkelman
Copy link
Contributor

tkelman commented Aug 19, 2016

namespace pollution - we should probably be putting these things in an Unchecked module.

@JaredCrean2
Copy link
Contributor

Sounds reasonable.

@KristofferC
Copy link
Member

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
sparse Sparse arrays
Projects
None yet
Development

No branches or pull requests

6 participants