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

Missed simplification opportunity for productmeasure #133

Open
cscherrer opened this issue Sep 5, 2023 · 8 comments
Open

Missed simplification opportunity for productmeasure #133

cscherrer opened this issue Sep 5, 2023 · 8 comments

Comments

@cscherrer
Copy link
Collaborator

In this measure

julia> d = productmeasure(fill(Lebesgue(), 5))
ProductMeasure([Lebesgue(ℝ), Lebesgue(ℝ), Lebesgue(ℝ), Lebesgue(ℝ), Lebesgue(ℝ)])

we have

julia> Base.issingletontype(eltype(marginals(d)))
true

But this guarantees we could instead have written it as a power measure. Let's update the powermeasure to account for this.

@oschulz
Copy link
Collaborator

oschulz commented Sep 6, 2023

Oh, nice!

@theogf
Copy link
Collaborator

theogf commented Sep 6, 2023

But you would lose type stability no? Can you infer the type just from the input arguments?

Unless you want to make PowerMeasure a special case of ProductMeasure?

@cscherrer
Copy link
Collaborator Author

cscherrer commented Sep 6, 2023

I think it will still be type-stable. When we call

productmeasure(::AbstractArray{T,N})

the rule can fire or not depending on issingletontype(T). I think it's enough for it to be a function of the type like this. Am I missing something?

@cscherrer
Copy link
Collaborator Author

Currently (in #120) we have

_generic_productmeasure_impl(mar::AbstractArray) = ProductMeasure(asmeasure.(mar))

We could change this to

function _generic_productmeasure_impl(mar::AbstractArray{T}) where {T}
    if Base.issingletontype(T)
        first(mar) ^ size(mar)
    else
        ProductMeasure(asmeasure.(mar))
    end
end

@cscherrer
Copy link
Collaborator Author

I guess we could even make it

if Base.issingletontype(T) || allequal(mar)

This requires walking through mar, and the fallback asmeasure.(mar) will require another traversal. It'd be better if we could fuse those. Shouldn't be too hard...

@cscherrer cscherrer linked a pull request Sep 6, 2023 that will close this issue
@cscherrer
Copy link
Collaborator Author

Keeping it simple for now. Added this as a test:

julia> productmeasure(fill(Lebesgue(), 5)) isa PowerMeasure
true

@oschulz
Copy link
Collaborator

oschulz commented Sep 7, 2023

Maybe add @inferred to the test?

@cscherrer
Copy link
Collaborator Author

Good idea! Done.

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 a pull request may close this issue.

3 participants