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

Add DiffResult for MArray, take 2 #35

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

gdalle
Copy link
Member

@gdalle gdalle commented Mar 19, 2024

Reverts #32

Copy link

codecov bot commented Mar 19, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 86.84%. Comparing base (91614bb) to head (e260e02).

Additional details and impacted files
@@           Coverage Diff           @@
##           master      #35   +/-   ##
=======================================
  Coverage   86.84%   86.84%           
=======================================
  Files           1        1           
  Lines          76       76           
=======================================
  Hits           66       66           
  Misses         10       10           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@gdalle gdalle marked this pull request as draft March 19, 2024 13:31
src/DiffResults.jl Outdated Show resolved Hide resolved
Co-authored-by: David Widmann <[email protected]>
@gdalle
Copy link
Member Author

gdalle commented Mar 19, 2024

How would you spot if there is one SArray among the Vararg?

@gdalle gdalle mentioned this pull request Mar 19, 2024
@devmotion
Copy link
Member

How would you spot if there is one SArray among the Vararg?

I'd define something like

# It would be safer to use `false` as the default fallback...
ismutablearray(::AbstractArray) = true
ismutablearray(::SArray) = false
ismutablearray(::Diagonal{T,SMatrix{T}}) = false
...

DiffResult(value::Number, derivs::Tuple{Vararg{Number}}) = ImmutableDiffResult(value, derivs)
function DiffResult(value::Union{Number,AbstractArray}, derivs::Tuple{Vararg{AbstractArray}})
    if all(ismutablearray, derivs)
        return MutableDiffResult(value, derivs)
    else
        return ImmutableDiffResult(value, derivs)
    end
end

@gdalle
Copy link
Member Author

gdalle commented Mar 19, 2024

Is it type-inferrable to do all(f, tuple) in the constructor?

@devmotion
Copy link
Member

devmotion commented Mar 19, 2024

I'm sure up to a certain extent 😄 But I haven't tested it. IIRC sometimes one can help the compiler by defining such checks recursively. And if the checks only depend on types (and instances are not needed), one could use Base.tuple_type_tail and Base.tuple_type_head.

But how many orders of derivatives do people realistically work with? So I think in most cases all should be fine.

@tpapp
Copy link

tpapp commented Mar 20, 2024

FWIW, I am not sure that mixing SArrays and mutable array types in a DiffResult is a use case we really need to support. For practical purposes, it is as likely to be a bug on the caller's side as anything else.

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

Successfully merging this pull request may close these issues.

3 participants