parthenon icon indicating copy to clipboard operation
parthenon copied to clipboard

`Remove` methods

Open Yurlungur opened this issue 1 year ago • 6 comments

Both MeshBlockData and SwarmContainer have Remove methods, which allow one to remove a field (either a variable or a particle). This strikes me as kind of counter to the parthenon model, where we describe what we want at the state descriptor level and then don't change things. I also don't think it's being used, so I'd like to elliminate it. Is anyone using these methods and would they object to removing them?

Pinging @pgrete and @lroberts36 . And especially @bprather because if anyone is ~~abusing~~ relying on this it's you. Already spoke to @brryan and he's on board.

Yurlungur avatar Mar 09 '23 03:03 Yurlungur

I am abusing this, actually! How did you guess?

The central problem this solves for me is I want a single simulation with two distinct phases:

  1. Clean up magnetic field with an elliptic solve
  2. Evolve fluid normally

Both require using Parthenon's MPI syncs, but need to sync different variables. Rather than mess with metadata after the fact, I just remove the variables I don't need from the container I'm using.

The appropriate solution would be to allow syncing sub-sets of variables on any given MPI call -- but for my particular case the sub-sets machinery would need to further be threaded through the solvers, so it would be quite a process.

bprather avatar Mar 09 '23 15:03 bprather

@bprather one reason I bring this up is I think the new flags machinery may not entirely clean up after itself. In particular I don't think the MeshblockData::flagsToVars_ object is cleaned up when remove is called.

As an aside, why can't you make a new container containing fewer variables?

Yurlungur avatar Mar 09 '23 15:03 Yurlungur

Hmm, maybe CopyFrom is all I need. Certainly for splitting off the solver step, I should be using that instead of copying and removing.

I think the issue is that by default the "base" block has all variables that were added in package initialization, I think. So I need to remove something from it at some point.

bprather avatar Mar 09 '23 15:03 bprather

I am a little confused, it looks like MeshBlockData::Remove is defined as

template <typename T>
void MeshBlockData<T>::Remove(const std::string &label) {
  throw std::runtime_error("MeshBlockData<T>::Remove not yet implemented");
}

on develop. Given this, I would suggest removing it.

@bprather did you implement this functionality on another branch?

lroberts36 avatar Mar 10 '23 00:03 lroberts36

You know, looking back I did "implement" it in my branch for KHARMA, and then just forgot. It's just one line, varMap_.erase(label);

...meaning it probably plays hell with memory, but I only needed to remove the MPI sync calls, which appeared to be enumerated from the varMap...

Anyway yes, I'm now definitely in favor of removing it entirely. I'll deal with switching to CopyFrom when I next pull develop.

bprather avatar Mar 10 '23 19:03 bprather

Sounds good. I'll remove these then when I do some other plumbing.

Yurlungur avatar Mar 10 '23 20:03 Yurlungur