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

The Data of my Block is not my Data #1220

Open
bprather opened this issue Jan 7, 2025 · 2 comments
Open

The Data of my Block is not my Data #1220

bprather opened this issue Jan 7, 2025 · 2 comments

Comments

@bprather
Copy link
Collaborator

bprather commented Jan 7, 2025

@hyerincho recently found a cluster of really subtle bugs in KHARMA, of the form:

void SomeFunction(MeshData *md)
{
    auto pmesh = md->GetMeshPointer();
    for (auto &pmb : pmesh->block_list) {
        auto rc = pmb->meshblock_data.Get();

This pattern is what I used to sort through mesh blocks on the host, in order to call a kernel on just a few blocks (e.g., for boundary conditions). However, it does not iterate through all MeshBlockData in md! It iterates through all MeshBlockData in "base"! What I wanted, of course, was auto rc = pmb->meshblock_data.Get(md->StageName());.

Clearly this is my bug, and maybe "git gud" is a valid answer. However, I think the interface is misleading in this case, and I'd like to revise it to clarify. To that end:

  1. Is there a cleaner iteration pattern which just gives you each MeshBlockData in a particular MeshData? I haven't run across Parthenon using one but I could easily have missed it. (I know there's NumBlocks() and GetBlock(i) but I considered the for-each loop a bit cleaner than that.)
    1a. (Should there be?)
  2. Should we maybe remove the no-argument Get() in favor of requiring a stage name? Or at least, require one if more than one stage exists in meshblock_data? And/or, should meshblock_data and mesh_data be renamed (perhaps mesh_data_collection?) considering they are a collection of objects and not just one?

Happy to do the (pretty easy, mostly docs) legwork on any changes folks are happy with. But this is inevitably going to require a ctrl-f update in all downstreams.

@bprather bprather changed the title The Data of myBlock is not myData The Data of my Block is not my Data Jan 7, 2025
@pgrete
Copy link
Collaborator

pgrete commented Jan 10, 2025

Does for (const auto &pmbd : md->GetAllBlockData() potentially do the trick?

@bprather
Copy link
Collaborator Author

Oooh, yes that's exactly the pattern I should be using! I'd missed that function re-reading the header... That solves (1).

I'd still love to remove Get(), and move Parthenon & examples to the new pattern where pmb : block_list still appears in the code so we encourage iterating over Data directly. Maybe I can open a PR and discuss there?

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

No branches or pull requests

2 participants