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 helper function bounding_box #880

Merged
merged 11 commits into from
Feb 6, 2024
Merged

Add helper function bounding_box #880

merged 11 commits into from
Feb 6, 2024

Conversation

lijas
Copy link
Collaborator

@lijas lijas commented Feb 2, 2024

I think it can be nice to add some grid utils. In this PR I am adding compute_bouding_box() , which I have recently needed for some pre-processing stuff.

Note. The current code is type unstable for some reason. Both max_vertex and min_vertex are both Core.Box. Anyone understands why?

@KnutAM
Copy link
Member

KnutAM commented Feb 2, 2024

Interesting problem - don't know why it is type unstable but could solve it if Tensors would support map

Base.map(f, iters::T...) where {T<:AbstractTensor} = T(map(f, Tensors.get_data.(iters)...))
function compute_bounding_box(grid::AbstractGrid{dim}) where {dim}
    T = get_coordinate_eltype(grid)
    min_vertex = Vec{dim,T}(i->T(+Inf))
    max_vertex = Vec{dim,T}(i->T(-Inf))
    for node in getnodes(grid)
        x = get_node_coordinate(node)
        max_vertex = map(max, max_vertex, x)
        min_vertex = map(min, min_vertex, x)
    end
    return min_vertex, max_vertex
end

Somehow defining an output tmp = max(max_vertex[1], x[1]) and only considering the first node, tmp was set as Any if max_vertex and min_vertex were updated and outputted, but type stable otherwise - so perhaps it is just inference giving up for some reason?

@lijas
Copy link
Collaborator Author

lijas commented Feb 2, 2024

I asked on slack helpdesk, and got this reply:

This will be slow, because Julia will need to pass the value boxed into the closure to make sure it gets the value of the right type.
This is a bug in Julia, because type inference could be used to determine that the box is unnecessary, but currently in Julia, the compiler step that creates the box happens before type inference, so it can't be optimised away (or so I'm told - I don't understand it).
The solution is:
Either, typeassert your variables so that the box have less impact on performance (but it'll still allocate)
Or, use a let statement

src/Grid/grid_utils.jl Outdated Show resolved Hide resolved
src/Ferrite.jl Outdated Show resolved Hide resolved
src/Grid/grid_utils.jl Outdated Show resolved Hide resolved
src/Grid/grid_utils.jl Outdated Show resolved Hide resolved
test/test_abstractgrid.jl Outdated Show resolved Hide resolved
test/test_grid_dofhandler_vtk.jl Outdated Show resolved Hide resolved
src/Grid/utils.jl Outdated Show resolved Hide resolved
src/Grid/utils.jl Outdated Show resolved Hide resolved
src/Grid/utils.jl Outdated Show resolved Hide resolved
Co-authored-by: Dennis Ogiermann <[email protected]>
@fredrikekre fredrikekre changed the title Add helper function compute_bouding_box Add helper function bouding_box Feb 6, 2024
@fredrikekre fredrikekre merged commit fa48d1e into master Feb 6, 2024
7 checks passed
@fredrikekre fredrikekre deleted the eb_gridutils branch February 6, 2024 13:00
@fredrikekre fredrikekre changed the title Add helper function bouding_box Add helper function bounding_box Feb 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants