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

Changing face to facet in variable names, struct fields, and docstrings #1123

Merged
merged 4 commits into from
Jan 7, 2025

Conversation

KnutAM
Copy link
Member

@KnutAM KnutAM commented Jan 5, 2025

Except direct field access of (fqr::FacetQuadratureRule).face_rules, this PR should not have any influence downstream AFAICS.

Copy link

codecov bot commented Jan 5, 2025

Codecov Report

Attention: Patch coverage is 99.25373% with 1 line in your changes missing coverage. Please review.

Project coverage is 93.57%. Comparing base (b8f986a) to head (c27cad6).
Report is 1 commits behind head on master.

Files with missing lines Patch % Lines
src/Quadrature/quadrature.jl 88.88% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##           master    #1123   +/-   ##
=======================================
  Coverage   93.57%   93.57%           
=======================================
  Files          39       39           
  Lines        6073     6073           
=======================================
  Hits         5683     5683           
  Misses        390      390           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@KnutAM KnutAM requested a review from termi-official January 6, 2025 20:40
@termi-official
Copy link
Member

Still some left

Grid/grid.jl:350:### Mesh interface ###
Grid/grid.jl:385:        facesets = nothing, # deprecated
Grid/grid.jl:389:    if facesets !== nothing
Grid/grid.jl:392:            for (key, set) in facesets
Grid/grid.jl:531:Returns all faces as `FacetIndex` in the set with name `setname`.
Grid/grid.jl:639:boundaryfunction(::Type{FaceIndex}) = faces
Grid/grid.jl:651:        #To be able to do a,b = faceidx
Grid/grid.jl:654:        # Necessary to check if, e.g. `(cellid, faceidx) in faceset`
Grid/grid.jl:688:    SurfaceOrientationInfo
Grid/grid.jl:693:Take for example the faces
Grid/grid.jl:699:which are rotated against each other by 90° (shift index is 1) or the faces

FEValues/FacetValues.jl:123:@inline function reinit!(fv::FacetValues, x::AbstractVector, face_nr::Int)
FEValues/FacetValues.jl:124:    return reinit!(fv, nothing, x, face_nr)
FEValues/FacetValues.jl:127:function reinit!(fv::FacetValues, cell::Union{AbstractCell, Nothing}, x::AbstractVector{Vec{dim, T}}, face_nr::Int) where {dim, T}
FEValues/FacetValues.jl:142:    @inbounds for (q_point, w) in pairs(getweights(fv.fqr, face_nr))
FEValues/FacetValues.jl:146:        weight_norm = weighted_normal(J, getrefshape(geo_mapping.ip), face_nr)

Queriev with ack -v "facet" | ack "face" and some manual filtering.

@KnutAM
Copy link
Member Author

KnutAM commented Jan 7, 2025

Thanks!
Fixed,

  • Grid/grid.jl:531:Returns all faces as `FacetIndex` in the set with name `setname`.
  • Grid/grid.jl:651: #To be able to do a,b = faceidx
  • Grid/grid.jl:654: # Necessary to check if, e.g. `(cellid, faceidx) in faceset`
  • All (and some more) in FacetValues.jl

In the grid constructor, it is correct to have face, also in the SurfaceOrientationInfo (Should formally now be FaceOrientationInfo and EdgeOrientationInfo IMO, and this should be changed before implemented and made public (if that's the plan).

There is also a default FaceIndex in BCValues that I saw now, but I'll do this in a separate PR since this PR aims to only do internal renaming (except the mentioned exception).

@KnutAM KnutAM merged commit 951ec47 into master Jan 7, 2025
16 checks passed
@KnutAM KnutAM deleted the kam/FixFacetValuesInternalNaming branch January 7, 2025 22:52
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.

2 participants