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

Fixing bug in boundary discrete model #651

Merged
merged 4 commits into from
Sep 7, 2021

Conversation

amartinhuertas
Copy link
Member

No description provided.

The current implementation of BoundaryDiscreteModelsTests.jl
does not allow for a different ordering of the cells than the one
in the model from which it is created
were actually not well defined. This commit defines appropiately this
set of tests.
in the portion the numbering of the faces of dimension d < D (as
before), but sticks into the user-provided ordering for the faces of
dimension d=D (this is the actual fix)
@codecov-commenter
Copy link

codecov-commenter commented Aug 31, 2021

Codecov Report

Merging #651 (fb22c9c) into master (b262130) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #651   +/-   ##
=======================================
  Coverage   87.72%   87.73%           
=======================================
  Files         134      134           
  Lines       14350    14360   +10     
=======================================
+ Hits        12589    12599   +10     
  Misses       1761     1761           
Impacted Files Coverage Δ
src/Geometry/DiscreteModelPortions.jl 100.00% <100.00%> (ø)

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 b262130...fb22c9c. Read the comment docs.

@fverdugo
Copy link
Member

fverdugo commented Sep 6, 2021

Hi @amartinhuertas, can you briefly comment on what the problem was?

@amartinhuertas
Copy link
Member Author

Hi @amartinhuertas, can you briefly comment on what the problem was?

Sure. The previous-to-the-fix function aimed to preserve the relative order in the numbering of the objects of the background model in the numbering of the objects filtered for the BoundaryDiscreteModel. This is not necessarily OK for the objects of the highest dimension, i.e., the cells in the BoundaryDiscreteModel, as you may want to change the numbering of the cells in BoundaryDiscreteModel (i.e., their relative order in the background model).

@amartinhuertas
Copy link
Member Author

@fverdugo ... FYI ... the following test (that I added on porpuse)

https://github.com/gridap/Gridap.jl/pull/651/files#diff-acafa9fbe3103748c87293ace08fb9618d888c62584ecac02e694f535a888badR63

was broken before the bug fix

@fverdugo
Copy link
Member

fverdugo commented Sep 7, 2021

Sure. The previous-to-the-fix function aimed to preserve the relative order in the numbering of the objects of the background model in the numbering of the objects filtered for the BoundaryDiscreteModel. This is not necessarily OK for the objects of the highest dimension, i.e., the cells in the BoundaryDiscreteModel, as you may want to change the numbering of the cells in BoundaryDiscreteModel (i.e., their relative order in the background model).

Ok. In any case BoundaryDiscreteModel is likely to be repaced by something else in the future to simplify the handling of mixed dimensional PDEs.

@fverdugo fverdugo merged commit 5e47263 into master Sep 7, 2021
@fverdugo fverdugo deleted the fixing_bug_in_boundary_discrete_model branch September 7, 2021 05:41
@amartinhuertas
Copy link
Member Author

In any case BoundaryDiscreteModel is likely to be repaced by something else in the future to simplify the handling of mixed dimensional PDEs.

Yes, please, :-)

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.

3 participants