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

Face coordinates #702

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

Face coordinates #702

wants to merge 2 commits into from

Conversation

lijas
Copy link
Collaborator

@lijas lijas commented May 10, 2023

I working on an example with contact, where it is nice to have some functionality for getting the coordinates of a face.

Up until this point, there has not really been a need for this in Ferrite since we have used FaceValues which uses all the coords/shape functions of the cell.

I am redefining the function getcoordinates(grid::AbstractGrid, face::FaceIndex) , but a doubt anyone has been using this.

for faceindex in contactfaces
    getcoordinates!(facecoords, grid, faceindex)
    ...
end

Note: draft PR, so I will add tests etc :)

@KnutAM
Copy link
Member

KnutAM commented May 10, 2023

Nice!
For ref: #664 deals with the names of these functions.
getcoordinates!(cellcoords, grid, faceindex) can be used for looping over faces and reiniting facevalues. (But this is quite unexpected in my opinion that it returns the cell coordinates, hence the renaming suggestions in the linked pr)


@inline function _getfacecoordinates!(x::Vector{Vec{dim,T}}, grid::Ferrite.AbstractGrid, cell::Ferrite.AbstractCell, lfaceid::Int) where {dim,T}
ip = default_interpolation(typeof(cell))
faceindices = Ferrite.facedof_indices(ip)[lfaceid]
Copy link
Member

Choose a reason for hiding this comment

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

Don't you want faces(cell) here instead?

Copy link
Collaborator Author

@lijas lijas May 11, 2023

Choose a reason for hiding this comment

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

faces only returns the vertices/corner-nodes of the cell (even for QuadraticHexahedrons etc). So we would need to update faces(cells) in that case...

Copy link
Member

Choose a reason for hiding this comment

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

I think we should not touch these definitions (i.e. faces, because they already give the expected description of the corresponding topological entity and this is what we actually need in most cases.

Copy link
Member

Choose a reason for hiding this comment

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

I should also note that AFAIK there is no hard guarantee that the returned tumple of facedof_indices must match the exact ordering of the face nodes.

Copy link
Member

@KnutAM KnutAM May 11, 2023

Choose a reason for hiding this comment

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

Would be nice to have a good interface for getting the dofs on a face, this is also used in this function:

function _add!(ch::ConstraintHandler, pdbc::PeriodicDirichlet, interpolation::Interpolation,

(when I needed it once I would have wanted such an interface, e.g. get_face_dofs[!]([dofinds], dh, faceindex))

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@termi-official I dont know what makes most sense: faces(QuadraticHexhedron) returning only the 4 corner nodes or all 9 nodes on the face.

But I think the cell-ordering has to follow facedof_indices right?, otherwise the dof-distrubtion would be wrong I think.

Copy link
Member

Choose a reason for hiding this comment

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

Note that the purpose of facedof_indices is different from facedof_interior_indices. We only need the latter during serial dof distribution. The latter is just a query of "give me every dof in the face", which is a cherry pick of what I need in the distributed assembly PR. I think it makes sense to somehow enforce that the order of the dofs is aligned with the subentity order on the corresponding cell, but I need to check exactly which invariants I will require for the dof synchronization in the new API. Does this clear up the confusion?

Copy link
Member

Choose a reason for hiding this comment

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

Not sure if I understand the first one correctly. faces(QuadraticHexhedron) should return the 6 faces with each face described by its corners. This is everything that we need to derive the relevant topological information as well as orientation information. The higher order nodes give you "just" the extra geometric information about how the element deforms on this specific face. But yea, we should implement some additional API to query higher order information for the geometry, even if it is just by enforcing a strict correspondence between the element ad the default interpolation (which I think Kim did vote against previously).

Copy link
Collaborator

@AbdAlazezAhmed AbdAlazezAhmed Jul 7, 2023

Choose a reason for hiding this comment

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

Just a reminder (not sure if it's needed here), Ferrite.facedof_indices is not defined for discontinuous interpolations. In case it's needed, Ferrite.dirichlet_facedof_indices was added in #729 . It might sound out of place here tho.

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.

5 participants