search

lanl / phoebus

Phifty One Ergs Blows Up A Star
BSD 3-Clause "New" or "Revised" License
46 stars 2 forks source link

Fix: No more MeshData parent pointers in torus history callbacks #209

Closed AstroBarker closed 7 months ago

AstroBarker commented 7 months ago

As pointed out in #208, there was improper use of MeshData pointers in the torus history callback functions. This was there in order to grab a couple of params, namely the event horizon xh and cutoff magnetization value sigma_cutoff. I've removed MeshData parent pointers and instead pass the relevant pieces into the functions, lambda captured into the callbacks.

AstroBarker commented 7 months ago

Radiation test currently failing, looks like

@AstroBarker while you already doing this, change this to sparse pack and we can delete my PR.

If someone wants to point me to the correct syntax for doing from MeshData, without pulling out the parent pointer, then I can do it. MeshData does not have access to resolved_packages on its own.

Yurlungur commented 7 months ago

Sorry it's a bit gross. We should really make it possible to just pass in MeshData.

  Mesh *pmesh = md->GetMeshPointer();
  auto &resolved_pkgs = pmesh->resolved_packages;
AstroBarker commented 7 months ago

Sorry it's a bit gross. We should really make it possible to just pass in MeshData.

  Mesh *pmesh = md->GetMeshPointer();
  auto &resolved_pkgs = pmesh->resolved_packages;

The GetMeshPointer is exactly what was removed in this PR. Was that not the issue?

Yurlungur commented 7 months ago

Maybe the code @mari2895 and I were looking at was old? The issue I saw was that MeshData was being passed into a function called on device.

mari2895 commented 7 months ago

Maybe the code @mari2895 and I were looking at was old? The issue I saw was that MeshData was being passed into a function called on device.

@Yurlungur No, it's from my PR (https://github.com/lanl/phoebus/pull/207) where I changed to sparse pack. It is not merged in main but I merged into my brach.

AstroBarker commented 7 months ago

Maybe the code @mari2895 and I were looking at was old? The issue I saw was that MeshData was being passed into a function called on device.

Can you point me to the function? I must have been confused. ReduceMassAccretionRate takes MeshData *md because it is the history callback function (or in the lambda that is the history callback function..) and those expect MeshData

mari2895 commented 7 months ago

Maybe the code @mari2895 and I were looking at was old? The issue I saw was that MeshData was being passed into a function called on device.

@Yurlungur that's exactly what I was doing, passing MeshData and you said it's wrong.

mari2895 commented 7 months ago

Maybe the code @mari2895 and I were looking at was old? The issue I saw was that MeshData was being passed into a function called on device.

Can you point me to the function? I must have been confused. ReduceMassAccretionRate takes MeshData *md because it is the history callback function (or in the lambda that is the history callback function..) and those expect MeshData

This is about CalcMassFlux argument. That's how I understood what @Yurlungur said that that function cannot take MeshData.

Yurlungur commented 7 months ago

I was probably unclear---my apologies. I'm very scatterbrained right now.

In @mari2895 's code, I saw a call to CalcMassFlux where the function was called inside a par_reduce and takes a MeshData<Real>* argument. This will fail because MeshData<Real>* is a pointer to host memory. When the pointer is copied to device, the pointer becomes invalid. Is this still in the develop branch?

AstroBarker commented 7 months ago

Maybe the code @mari2895 and I were looking at was old? The issue I saw was that MeshData was being passed into a function called on device.

Can you point me to the function? I must have been confused. ReduceMassAccretionRate takes MeshData *md because it is the history callback function (or in the lambda that is the history callback function..) and those expect MeshData

This is about CalcMassFlux argument. That's how I understood what @Yurlungur said that that function cannot take MeshData.

I think I see now. The CalcMassFlux code in main does not take MeshData. That behavior is in a branch and is not merged. I think we can close #208 ? As well as this PR. Let me know.

Yurlungur commented 7 months ago

Yeah we can close both. Sorry for the confusion @AstroBarker .