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

[WIP] Revise ρ_upper_bound #778

Closed
wants to merge 3 commits into from
Closed

[WIP] Revise ρ_upper_bound #778

wants to merge 3 commits into from

Conversation

schillic
Copy link
Member

@schillic schillic commented Oct 12, 2018

I decided to end this mess with passing upper_bound around. Now we have ρ_upper_bound for all relevant lazy operations. For all other set types we use the default ρ. The only remainder is Intersection, which we can rectify in another PR.
However, with the current settings we cannot run the bouncing ball model anymore...

@schillic schillic changed the title Revise ρ_upper_bound [WIP] Revise ρ_upper_bound Oct 12, 2018
@schillic schillic mentioned this pull request Oct 13, 2018
@schillic schillic force-pushed the schillic/refactor_ρ branch from f789180 to 17ddc85 Compare October 13, 2018 08:49
@schillic schillic force-pushed the schillic/refactor_ρ branch from 17ddc85 to b2cf366 Compare October 13, 2018 09:01
@schillic
Copy link
Member Author

schillic commented Oct 13, 2018

Now everything should work again. Note, however, that the execution of ρ is now less efficient, which is not good 😞

master:

julia> @time ρ(d, X+(X∩H))
  0.000178 seconds (474 allocations: 42.469 KiB)
julia> @time ρ(d, X+(X+X))
  0.000016 seconds (13 allocations: 816 bytes)

this branch:

julia> @time ρ(d, X+(X∩H))
  0.000183 seconds (582 allocations: 50.906 KiB)
julia> @time ρ(d, X+(X+X))
  0.000009 seconds (22 allocations: 1.375 KiB)

I guess this is due to the additional keyword arguments (even if they are not used).


There might be a solution by defining additional ρ/ρ_upper_bound methods without keyword arguments (basically the old implementation). An idea is to define

ρ_args(kwargs...) = (d, X) -> ρ(d, X, kwargs...)
ρ_upper_bound_args(kwargs...) = (d, X) -> ρ_upper_bound(d, X, kwargs...)

globally and then instead of passing ρ resp. ρ_upper_bound to ρ_helper, in the two new implementations we just pass the respective function object and remove the kwargs... from ρ_helper.
Example:

function ρ_helper(d, X, ρ_rec)
    ...
    ρ_rec(d2, X2)
    ...
end
function ρ(d, X)
    return ρ_helper(d, X, ρ)
end
function ρ(d, X; kwargs...)
    return ρ_helper(d, X, ρ_args(kwargs...))
end
function ρ_upper_bound(d, X)
    return ρ_helper(d, X, ρ_upper_bound)
end
function ρ_upper_bound(d, X; kwargs...)
    return ρ_helper(d, X, ρ_upper_bound_args(kwargs...))
end

Actually, now I realize that we might be able to outsource all these small functions and only need to implement ρ_helper for each type. The only downside would be that we lose the docstrings, but usually they are just copy-paste anyway.

@schillic schillic force-pushed the schillic/refactor_ρ branch from 42f7aec to 3fbf13a Compare October 13, 2018 12:30
@schillic
Copy link
Member Author

New version in this branch:

julia> @time ρ(d, X+(XH))
  0.000177 seconds (476 allocations: 42.625 KiB)

julia> @time ρ(d, X+(X+X))
  0.000008 seconds (17 allocations: 1008 bytes)

@schillic
Copy link
Member Author

Closing in favor of #779.

@schillic schillic closed this Oct 14, 2018
@schillic schillic deleted the schillic/refactor_ρ branch October 14, 2018 06:10
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.

1 participant