Skip to content

Conversation

@JamesMcClung
Copy link
Collaborator

@JamesMcClung JamesMcClung commented May 5, 2025

This makes it safe to do mprts.accessor()[p] without having to worry about the result of mprts.accessor() being dropped.

This was happening in #354, in particular.

it was possible for a ConstAccessorSimple to be dropped before its
corresponding ConstAccessorPatchSimple
@germasch
Copy link
Contributor

germasch commented May 5, 2025

I think you found the reason why this was so kinda complicated in the first place, with the Accessor replicating .data() and .size() -- because I think for the CUDA version, the accessor actually works differently, by copying the data to the host first, and in that case once the accessor is gone, the copied data is gone, and this just isn't going to work. So I think the better way forward here is to just work around the problem in your test by keeping the accessor around, and putting "make accessor safe" onto your list of things to do later (if ever).

While I introduced the accessor stuff for a reason (likely actually because of CUDA), I'd actually rather get rid of this complexity again in some future vision of CPU/GPU compatability more akin to how gtensor does stuff.

Copy link
Contributor

@germasch germasch left a comment

Choose a reason for hiding this comment

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

I guess there isn't actually a "reject" option for PR reviews, but well, I think that's my choice.

@JamesMcClung JamesMcClung deleted the pr/patch-accessor-stores-mprts branch May 5, 2025 18:47
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