- 
                Notifications
    
You must be signed in to change notification settings  - Fork 34
 
          fix: fix comparison involving NaN
          #334
        
          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
Conversation
        
          
                test/commutative/comparison.jl
              
                Outdated
          
        
      | @test isequal(NaN * x, NaN * x + 0) | ||
| @test !=(NaN * x, NaN * x) | ||
| @test !=(NaN * x + 0, NaN * x + 0) | ||
| @test !=(NaN * x, NaN * x + 0) | 
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.
Same comment for DynamicPolynomials, could you check that it's allocation free as well ?
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.
Fixed in 6d3e7f3
| @test !=(NaN * x, NaN * x) | ||
| @test !=(NaN * x + 0, NaN * x + 0) | ||
| @test !=(NaN * x, NaN * x + 0) | ||
| end | 
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.
Missing checks for RationalPoly it seems
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.
Fixed in 3cb3f49
| 
           Thanks ! Just a few minor comments but it looks good  | 
    
| 
           You can ignore the failure on nightly so just fix the format with JuliaFormatter@v2  | 
    
3cb3f49    to
    e5da00a      
    Compare
  
    | 
           Addressed the formatting issues  | 
    
| 
           Bump 😅 is there anything else left here?  | 
    
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.
Looks good, thanks!
Referring to JuliaAlgebra/DynamicPolynomials.jl#177
Some of the issues are fixed by JuliaAlgebra/DynamicPolynomials.jl#178. The rest require the changes in this PR.