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

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

Closed
wants to merge 2 commits into from

Conversation

AstroBarker
Copy link
Collaborator

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 AstroBarker added the bug Something isn't working label Mar 18, 2024
@AstroBarker AstroBarker changed the title Fix: No more MeshData pointers in torus history callbacks Fix: No more MeshData parent pointers in torus history callbacks Mar 18, 2024
Copy link
Collaborator

@mari2895 mari2895 left a comment

Choose a reason for hiding this comment

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

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

@AstroBarker
Copy link
Collaborator Author

AstroBarker commented Mar 18, 2024

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
Copy link
Collaborator

Yurlungur commented Mar 18, 2024

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
Copy link
Collaborator Author

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
Copy link
Collaborator

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
Copy link
Collaborator

mari2895 commented Mar 18, 2024

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 (#207) where I changed to sparse pack. It is not merged in main but I merged into my brach.

@AstroBarker
Copy link
Collaborator Author

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
Copy link
Collaborator

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
Copy link
Collaborator

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
Copy link
Collaborator

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
Copy link
Collaborator Author

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
Copy link
Collaborator

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

@Yurlungur Yurlungur closed this Mar 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants