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

Simplify extension of AbstractGrid #579

Closed
wants to merge 8 commits into from
Closed

Simplify extension of AbstractGrid #579

wants to merge 8 commits into from

Conversation

KnutAM
Copy link
Member

@KnutAM KnutAM commented Jan 9, 2023

I went through the codebase and replaced direct field access when reading with already defined get functions.
No usage change, but reduces the number of functions required to define an AbstractGrid,
see the updated example and test.

@KnutAM
Copy link
Member Author

KnutAM commented Jan 9, 2023

I think I've covered all places in the src folder where AbstractGrid input is allowed (except in close(::MixedDofHandler) as it gets removed in #577)

Feels like we should have something like get_node_coordinate(grid, nodeid) as I started calling getcoordinates(getnodes(grid, nodeid)) a lot, but I think adding new functions should be kept to a separate pr.

And getcoordinates is a bit tricky as it returns node coordinates if given a Node, but the cell coordinates for all other cases, even getcoordinates(grid, fi::FaceIndex), as was pointed out on slack some time ago...

@codecov-commenter
Copy link

codecov-commenter commented Jan 9, 2023

Codecov Report

Patch coverage: 95.74% and project coverage change: +0.09 🎉

Comparison is base (b019eea) 92.66% compared to head (b18025f) 92.75%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #579      +/-   ##
==========================================
+ Coverage   92.66%   92.75%   +0.09%     
==========================================
  Files          30       30              
  Lines        4483     4484       +1     
==========================================
+ Hits         4154     4159       +5     
+ Misses        329      325       -4     
Impacted Files Coverage Δ
src/L2_projection.jl 100.00% <ø> (ø)
src/Grid/grid.jl 91.92% <92.85%> (+1.04%) ⬆️
src/Dofs/ConstraintHandler.jl 94.67% <100.00%> (ø)
src/Dofs/MixedDofHandler.jl 91.36% <100.00%> (ø)
src/Export/VTK.jl 90.16% <100.00%> (ø)
src/iterators.jl 92.95% <100.00%> (ø)

... and 1 file with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@termi-official
Copy link
Member

The last commit is a bit problematic, because it potentially changes the complexity of the call.

@KnutAM
Copy link
Member Author

KnutAM commented Jan 10, 2023

The last commit is a bit problematic, because it potentially changes the complexity of the call.

Do you mean for some special abstract grid type?
I first changed, e.g. Tx = typeof(first(grid.nodes).x) to Tx = typeof(getcoordinates(getnodes(grid, 1))), but then went back to using first to be fully compatible (in case of offset arrays): Tx = typeof(getcoordinates(first(getnodes(grid))))

@termi-official
Copy link
Member

getnodes(grid) potentially allocates some big array (e.g. for some AMR grids) while getnodes(grid, 1) may not.

@KnutAM
Copy link
Member Author

KnutAM commented Jan 10, 2023

Fair point, and could be fixed by this PR if you think that is better for distributed grids.
I can only see a use for offset arrays with distributed grids anyways (but wouldn't indexing by 1 give a potential problem there. Or are the nodes on each core indexed by a local node index?)

A more general approach (outside the scope of this pr) could be to use EndpointRanges.jl and do
Tx = typeof(getcoordinates(getnodes(grid, ibegin))) which I find quite cool that it works:)

Edit: And for defining new functions (in separate pr), it could make sense to have a getcoordinatetype(grid) matching getcelltype(grid)

@termi-official
Copy link
Member

termi-official commented Jan 10, 2023

Indeed, a separate function getcoordinatetype definitely makes most sense. Lets do it like this.

To quickly comment on the distributed assembly, in the current design we have 3 (not yet well-separated) layers. First, there is the local layer, where local manipulations work locally as expected and we have process local indices. Then we have some transitional layer which maps between local and global entities and we have the synchronization layer, which synchronizes global entities. This e.g. allows to switch between distributed assembly and the current version by just changing three lines of code (exchange the grid, dof handler and assembler).

Edit: To quickly get to the point, the current design guarantees that local operations works as expected in distributed assembly. We can discuss this one separately in the distributed assembly PR if you have concerns or suggestions.

@KnutAM
Copy link
Member Author

KnutAM commented Jan 10, 2023

OK, great! Then I'll leave those changes to a "new functions" pr!
Regarding the distributed assembly, that sounds very cool that such small changes are required!

@KnutAM KnutAM closed this Jul 25, 2023
@KnutAM KnutAM deleted the kam/AbstractGrid branch July 25, 2023 07:10
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