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

Update some missing Lagrange docs #889

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

Conversation

termi-official
Copy link
Member

No description provided.

Copy link

codecov bot commented Mar 1, 2024

Codecov Report

Attention: Patch coverage is 0% with 5 lines in your changes are missing coverage. Please review.

Project coverage is 93.30%. Comparing base (c33a191) to head (1cd9a82).

Files Patch % Lines
src/Grid/grid.jl 0.00% 5 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #889      +/-   ##
==========================================
- Coverage   93.69%   93.30%   -0.39%     
==========================================
  Files          36       36              
  Lines        5438     5440       +2     
==========================================
- Hits         5095     5076      -19     
- Misses        343      364      +21     

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

Copy link
Member

@KnutAM KnutAM left a comment

Choose a reason for hiding this comment

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

Nice!
Just some formatting fix suggestions while we are at it...

docs/src/topics/degrees_of_freedom.md Outdated Show resolved Hide resolved
docs/src/topics/degrees_of_freedom.md Outdated Show resolved Hide resolved
docs/src/topics/grid.md Outdated Show resolved Hide resolved
docs/src/topics/grid.md Outdated Show resolved Hide resolved
Co-authored-by: Knut Andreas Meyer <knutam@gmail.com>
@termi-official termi-official marked this pull request as draft March 4, 2024 09:39
@termi-official termi-official marked this pull request as ready for review June 3, 2024 14:48
@termi-official
Copy link
Member Author

I noticed that on master some things can be now done in a better way (i.e. without relying on assumptions taken between the interpolation and cell) so I updated it. Also some dispatches were missing, which I added for completeness of the API.

@termi-official
Copy link
Member Author

I just saw that #935 has quite a bit of overlap here, so we need to decide on a merge order.

@termi-official
Copy link
Member Author

CI failure seems to be the github CDN or CI servers choking

ERROR: LoadError: LoadError: LoadError: Operation too slow. Less than 1 bytes/sec transferred the last 20 seconds while requesting https://raw.githubusercontent.com/Ferrite-FEM/Ferrite.jl/gh-pages/assets/transient_heat.gif

Copy link
Member

@KnutAM KnutAM left a comment

Choose a reason for hiding this comment

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

LGTM if if removing changes in grid.md as discussed on Slack, and fixes the comments

src/Grid/grid.jl Outdated Show resolved Hide resolved
docs/src/devdocs/elements.md Outdated Show resolved Hide resolved

Each `AbstractCell` type has a unique geometric interpolation describing its geometry.
This function returns that interpolation.
"""
geometric_interpolation(::AbstractCell)
geometric_interpolation(::T) where T <: AbstractCell = geometric_interpolation(T)
Copy link
Member

Choose a reason for hiding this comment

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

Missing test

Comment on lines +14 to +16
Please note that if your cell holds a single tuple called `nodes` to carry the node numbers, then
this method will work automatically. Almost all functions work automatically with the information
provided by the reference element.
Copy link
Member

Choose a reason for hiding this comment

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

Already mentioned in the docstring

Suggested change
Please note that if your cell holds a single tuple called `nodes` to carry the node numbers, then
this method will work automatically. Almost all functions work automatically with the information
provided by the reference element.

Comment on lines +18 to +19
Please note that if you want to implement a custom element which does not have a reference shape,
then the relevant functions below need to be dispatched.
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this is possible, AbstractCell{RT} requires RT<:AbstractRefShape?

Suggested change
Please note that if you want to implement a custom element which does not have a reference shape,
then the relevant functions below need to be dispatched.

Copy link
Member Author

Choose a reason for hiding this comment

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

struct MyFancyDynamicCell
    nodes::Vector{Int}
end

?

Copy link
Member

Choose a reason for hiding this comment

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

But that won't fit in the grid which requires

struct MyFancyDynamicCell <: AbstractCell{RefShape}
    nodes::Vector{Int}
end

?

Copy link
Member

Choose a reason for hiding this comment

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

And I guess lot of the codebase assumes that the refshape can be determined only by the type, so not sure how an arbitrary polygon element would work. Do we already have an issue for this? Otherwise, that would be good.

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

But I would leave this issue for later, as it does not have any priority for me.

Comment on lines +160 to +162
@inline reference_faces(::AbstractCell{refshape}) where refshape = reference_faces(refshape)
@inline reference_edges(::AbstractCell{refshape}) where refshape = reference_edges(refshape)
@inline reference_vertices(::AbstractCell{refshape}) where refshape = reference_vertices(refshape)
Copy link
Member

Choose a reason for hiding this comment

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

Use here?

@inline reference_faces(::AbstractCell{refshape}) where refshape = reference_faces(refshape)
@inline reference_edges(::AbstractCell{refshape}) where refshape = reference_edges(refshape)
@inline reference_vertices(::AbstractCell{refshape}) where refshape = reference_vertices(refshape)
@inline reference_facets(::AbstractCell{refshape}) where refshape = reference_facets(refshape)
Copy link
Member

Choose a reason for hiding this comment

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

Missing test

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.

None yet

3 participants