Skip to content

Adding reset_data to xbuffer_adaptor and reset_buffer to *adaptor to replace the pointer without any reallocation.#2521

Merged
JohanMabille merged 1 commit intoxtensor-stack:masterfrom
tdegeus:pointer
May 5, 2022
Merged

Adding reset_data to xbuffer_adaptor and reset_buffer to *adaptor to replace the pointer without any reallocation.#2521
JohanMabille merged 1 commit intoxtensor-stack:masterfrom
tdegeus:pointer

Conversation

@tdegeus
Copy link
Copy Markdown
Member

@tdegeus tdegeus commented Apr 26, 2022

Checklist

  • The title and commit message(s) are descriptive.
  • Small commits made to fix your PR have been squashed to avoid history pollution.
  • Tests have been added for new features or bug fixes.
  • API of new functions and classes are documented.

Description

Fixes #2519

@tdegeus
Copy link
Copy Markdown
Member Author

tdegeus commented Apr 26, 2022

Grateful for your feedback @emmenlau . I'm not sure that I like the name of the method, happy to take suggestions.

@tdegeus tdegeus added this to the v0.24.2 milestone Apr 26, 2022
@tdegeus
Copy link
Copy Markdown
Member Author

tdegeus commented Apr 26, 2022

Restarting CI

@emmenlau
Copy link
Copy Markdown
Contributor

Thanks a lot for the nice work @tdegeus ! Could you also take a look, @vakokako ?

About the name of the method, I find it also difficult to come up with a good suggestion. What about setdata() or setptr()? Or just ptr() if it can be used as a setter and getter? Just tossing ideas...

@vakokako
Copy link
Copy Markdown

vakokako commented May 2, 2022

Looks good. The problem is it's not really discoverable with xbuffer_storage and I had to dive into the code to really find this method. Also would be still nice to have this functionality implemented in a view as the intended purpose of the xt::adapt is wrapping non-xtensor containers rather than providing a movable view onto the existing xtensor container.

@tdegeus
Copy link
Copy Markdown
Member Author

tdegeus commented May 2, 2022

Thanks for your feedback @vakokako .

The problem is it's not really discoverable with xbuffer_storage and I had to dive into the code to really find this method.

Do you mean documentation is lacking? Do you think that this PR sufficiently helps in that direction?

Also would be still nice to have this functionality implemented in a view as the intended purpose of the xt::adapt is wrapping non-xtensor containers rather than providing a movable view onto the existing xtensor container.

I see your point. Though for a view I don't really have a good syntax in mind. Anyway it's a separate enhancement I guess. Would you be willing to open an issue on that, with a suggested API?

@vakokako
Copy link
Copy Markdown

vakokako commented May 3, 2022

Do you mean documentation is lacking? Do you think that this PR sufficiently helps in that direction?

It's just hidden under the .storage() method, while it could be the direct method of the adapted container like so:

auto b = xt::adapt(&a.flat(0), std::array<size_t, 2>{2, 2});
b.replace(&a.flat(1)); // maybe could be called `move`?

The types in xtensor are already hidden enough, without diving into code I wouldn't even know what type storage() is, that it has also some size, etc.

But I think this is a minor remark, thanks for implementing this pr!

@tdegeus
Copy link
Copy Markdown
Member Author

tdegeus commented May 4, 2022

@JohanMabille Can you check that the edit is what you had in mind?

@tdegeus tdegeus changed the title Adding replace to xbuffer_adaptor to replace the pointer without any reallocation. Adding reset_data to xbuffer_adaptor and reset_buffer to *adaptor to replace the pointer without any reallocation. May 4, 2022
…tor` to replace the pointer without any reallocation.
@JohanMabille
Copy link
Copy Markdown
Member

Awesome, thanks!

@JohanMabille JohanMabille merged commit 1412daa into xtensor-stack:master May 5, 2022
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.

Replace pointer for a tensor that does not own data

4 participants