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

Extend data access API #169

Open
wants to merge 26 commits into
base: main
Choose a base branch
from
Open

Extend data access API #169

wants to merge 26 commits into from

Conversation

benegee
Copy link
Collaborator

@benegee benegee commented Feb 22, 2024

New

  • trixi_ndofs(int handle);
  • trixi_ndofsglobal(int handle);
  • trixi_ndofselement(int handle);
  • trixi_nnodes(int handle);
  • trixi_load_primitive_vars(int handle, int variable_id, double * data);
  • trixi_load_node_reference_coordinates(int handle, double* node_coords);
  • trixi_load_node_weights(int handle, double* node_weights);

Breaking changes

  • trixi_nelementsglobal(int handle); (was trixi_nelements_global(int handle);)
  • trixi_load_element_averaged_primitive_vars(int handle, int variable_id, double * data); (was trixi_load_cell_averages(double * data, int handle);

Conventions

Adopt https://trixi-framework.github.io/Trixi.jl/stable/conventions/#Cells-vs.-elements-vs.-nodes. For libtrixi, avoid "cell" in favor of "element".

Related #43

add parameter index and only get averaged of the variable at position index
Copy link
Member

@sloede sloede left a comment

Choose a reason for hiding this comment

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

Looks sensible to me. Just note that changing the public API requires a breaking release. That's OK, of course.

Alternatively, you could consider to only extend the API, e.g., by adding a new trixi_load_cell_average or trixi_load_cell_averages_single function. That has the advantage that you can still decide to introduce a breaking change later. It has the downside, that you need more tests.

Copy link

codecov bot commented Feb 23, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 82.76%. Comparing base (acaf98e) to head (66ee664).

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #169      +/-   ##
==========================================
+ Coverage   80.27%   82.76%   +2.48%     
==========================================
  Files          19       19              
  Lines         715      818     +103     
  Branches       50       50              
==========================================
+ Hits          574      677     +103     
  Misses        137      137              
  Partials        4        4              
Flag Coverage Δ
unittests 82.76% <100.00%> (+2.48%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

@benegee benegee requested a review from sloede February 26, 2024 15:07
Copy link
Member

@sloede sloede left a comment

Choose a reason for hiding this comment

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

Looks mostly good to me, but I think there are some structural decisions to be made (or maybe they have been made but I am just not aware 😬)

LibTrixi.jl/src/LibTrixi.jl Outdated Show resolved Hide resolved
LibTrixi.jl/src/LibTrixi.jl Outdated Show resolved Hide resolved
LibTrixi.jl/src/LibTrixi.jl Outdated Show resolved Hide resolved
LibTrixi.jl/src/api_c.jl Outdated Show resolved Hide resolved
LibTrixi.jl/src/api_c.jl Outdated Show resolved Hide resolved
LibTrixi.jl/src/api_c.jl Outdated Show resolved Hide resolved
LibTrixi.jl/src/api_c.jl Outdated Show resolved Hide resolved
LibTrixi.jl/src/api_c.jl Outdated Show resolved Hide resolved
LibTrixi.jl/src/api_c.jl Outdated Show resolved Hide resolved
src/api.c Outdated Show resolved Hide resolved
@benegee
Copy link
Collaborator Author

benegee commented Mar 7, 2024

Say I want to stay as close to Trixi.jl as possible and have read
https://trixi-framework.github.io/Trixi.jl/stable/conventions/#Cells-vs.-elements-vs.-nodes
would there be any place in libtrixi where "cell" should be used?

@sloede
Copy link
Member

sloede commented Mar 8, 2024

Say I want to stay as close to Trixi.jl as possible and have read https://trixi-framework.github.io/Trixi.jl/stable/conventions/#Cells-vs.-elements-vs.-nodes would there be any place in libtrixi where "cell" should be used?

IMHO only when talking about mesh-related operations that are independent of the solver. For example, querying the number of cells is usually not something you need, since you are rather interested in the actual elements (i.e., cells with solver data attached to it), which might or might not have a one-to-one relation with the cells.

From the top of my head, I don't see many occasions where the word "cell" would come up as part of an API name.

@benegee
Copy link
Collaborator Author

benegee commented Mar 8, 2024

Great! Thanks for your hints!

This is now totally cell-free. And totally breaking.

@benegee benegee requested a review from sloede March 11, 2024 09:56
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