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

Allow evaluating FE functions at arbitrary points #523

Merged
merged 68 commits into from
Jun 2, 2021

Conversation

eschnett
Copy link
Contributor

I'm looking for feedback, in particular:

I'm aware that the code is still quite restrictive (only handles SingleFieldFEFunctions with simplicial meshes), and that it uses linear interpolation instead of actually evaluating the basis functions.

Will eventually close #425, I hope.

This is a first cut only, still very inefficient and restrictive.
@ericneiva
Copy link
Member

Hi, @eschnett,

Where is @ericneiva's branch that is mentioned in #425?

I have answered you in #425. Cheers!

@fverdugo
Copy link
Member

I'm using my existing package Bernstein.jl, but only call a single function in it. Should I instead import the respective code?

Which function do you need? Perhaps a gridap equivalent is already there.

What mechanism should I use to cache the calculated kdtree? Can I somehow attach it to the triangulation?

I would use the caching mechanism we have in gridap.

I.e., implement return_cache(f::CellField,x::Point) function. The result of this function, is then the first argument in evaluate!(cache,f::CellField,x::Point). Once these two functions are working you can efficiently evaluate the CellField in a vector of points by creating the cache only once:

points = ... # Vector{Point{D,Float64}}
uh = ... # CellField
cache = return_cache(uh,testitem(points)) # Generate the cache. i.e., do the setup
for x in points
  uh_at_x = evaluate!(cache,uh,x)
end

I'm aware that the code is still quite restrictive (only handles SingleFieldFEFunctions with simplicial meshes), and that it uses linear interpolation instead of actually evaluating the basis functions.

Yes, the final implementation should only rely on the CellField interface.

@eschnett
Copy link
Contributor Author

The function I'm using converts a point in a simplex from Cartesian to barycentric coordinates so that I know whether it is inside the simplex or not. This is necessary because the k-d tree search returns the nearest vertex to a give point, and I then have to search all neighbouring simplices. Is there a function for contained-ness?

@fverdugo
Copy link
Member

Perhaps we can consider this package

https://github.com/JuliaGeometry/GeometricalPredicates.jl

It has no dependencies, so including it is not harmful

@eschnett
Copy link
Contributor Author

That package only supports 2d and 3d simplices. I'll copy the respective code – it's not much, essentially just constructing and inverting a small matrix.

@notimplemented """\n
Evaluation of a CellField at a given Point is not implemented yet.
cache::CellFieldCache

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could probably encapsulate this code in a function and do dispatching with respect to the triangulation type. The code below is for an unstructured (simplicial) mesh. We could also consider the case for a structured Cartesian mesh (which is much easier, no tree involved).

@santiagobadia
Copy link
Member

I'm using my existing package Bernstein.jl, but only call a single function in it. Should I instead import the respective code?

Since this is unrelated to Berstein polynomials I would rather add the code here (I guess it is not much)

Is there a function for contained-ness?

No, I am not sure what is the best geometrical predicate for determining whether a a point belongs to an n-simplex. The one you have implemented seems a good option.

@codecov-io
Copy link

codecov-io commented Jan 27, 2021

Codecov Report

Merging #523 (73672a3) into master (923852a) will increase coverage by 0.15%.
The diff coverage is 95.52%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #523      +/-   ##
==========================================
+ Coverage   86.37%   86.53%   +0.15%     
==========================================
  Files         145      145              
  Lines       11386    11446      +60     
==========================================
+ Hits         9835     9905      +70     
+ Misses       1551     1541      -10     
Impacted Files Coverage Δ
src/CellData/CellData.jl 100.00% <ø> (ø)
src/CellData/CellFields.jl 80.22% <ø> (+0.60%) ⬆️
src/Geometry/CartesianGrids.jl 100.00% <ø> (+6.25%) ⬆️
src/MultiField/MultiField.jl 100.00% <ø> (ø)
src/MultiField/MultiFieldCellFields.jl 88.23% <94.11%> (+17.64%) ⬆️
src/Fields/AffineMaps.jl 60.52% <100.00%> (+1.06%) ⬆️
src/Geometry/UnstructuredGrids.jl 100.00% <100.00%> (ø)
src/TensorValues/Operations.jl 90.52% <100.00%> (+0.42%) ⬆️
src/Fields/FieldArrays.jl 91.37% <0.00%> (+0.65%) ⬆️
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 923852a...f8998e3. Read the comment docs.

@eschnett eschnett marked this pull request as ready for review April 30, 2021 20:20
Copy link
Member

@fverdugo fverdugo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @eschnett. Thanks for the PR!

Please, see my comments below 👇

test/FieldsTests/runtests.jl Outdated Show resolved Hide resolved
src/MultiField/MultiFieldCellFields.jl Outdated Show resolved Hide resolved
test/MultiFieldTests/MultiFieldCellFieldsTests.jl Outdated Show resolved Hide resolved
@santiagobadia
Copy link
Member

Hi @fverdugo @eschnett

@Balaje is updating @eschnett with master since he needs something similar for his GSoC project. @Balaje can do this work and update the PR, ok?

@eschnett
Copy link
Contributor Author

@santiagobadia Are you volunteering @Balaje to update the PR? Let me know if you need help or get stuck. I'd be happy to do this work myself.

@Balaje
Copy link
Contributor

Balaje commented Jun 1, 2021

Hi @eschnett

I have already made a PR to your repo with the suggested changes. I see that you have merged the PR.

@santiagobadia @ericneiva I have updated test/CellDataTests/CellFieldsTests.jl with some more tests. Should I submit another pull request to @eschnett's branch?

@santiagobadia
Copy link
Member

Hi @Balaje

Yes, if you make changes in @eschnett fork, please send a PR to @eschnett repo. When @eschnett accepts the PR, this one updates automatically, I think.

Hopefully, we are close to accept the PR. Sorry @eschnett for the mess. Thanks @Balaje for the work.

Copy link
Member

@fverdugo fverdugo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @eschnett @Balaje ! Thanks for the changes. I still have a couple of minor comments.

src/MultiField/MultiField.jl Show resolved Hide resolved
src/MultiField/MultiFieldCellFields.jl Outdated Show resolved Hide resolved
Copy link
Member

@fverdugo fverdugo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @Balaje @eschnett !

Still some more minor comments.

@@ -57,6 +61,10 @@ export CellDof
export get_normal_vector
export get_cell_measure

export make_inverse_table
export point_to_cell!
Copy link
Member

@fverdugo fverdugo Jun 2, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are exporting point_to_cell! but the function to generate its cache object seems to be private.

Here, there are 2 options:

  1. Do not export point_to_cell! and rename it to _point_to_cell! so that it is clear that it's private.
  2. Make public also the mechanism to generate the cache. In this case, follow the API of the abstract type Map just to be consistent with other parts of the code.
struct PointToCellMap{T<:Triangulation} <: Map
  trian::T
end

return_cache(k::PointToCellMap,x::Point) = ...
evaluate!(cache,k::PointToCellMap,x::Point) = ...

@santiagobadia
Copy link
Member

I have created an issue with the last minor comment. I proceed to merge this PR.

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.

Make FEFunction objects callable (as functions of coordinates)
7 participants