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

SF#411 relax reshape() constraints #217

Open
mohawk2 opened this issue Apr 9, 2018 · 2 comments
Open

SF#411 relax reshape() constraints #217

mohawk2 opened this issue Apr 9, 2018 · 2 comments

Comments

@mohawk2
Copy link
Member

mohawk2 commented Apr 9, 2018

@devel-chm writes:

To avoid a bug in the reshape() routine, it no longer will operate on a PDL with data flow active. This avoids an interaction between reshape() and its dimension mangling and mv() which can re-order dimensions in such a way that the reshape() implementation ends up indexing out of the piddle data. This was motivated by the fact that reshape() is mainly for turning a contiguous block of data into an equivalent multidimensional piddle and so applying it to a piddle with scrambled axis orders doesn't really make sense.

However, many uses of reshape() are essentially shorthand for a sequence of splitdim() operations and it would be nice if this were allowed. I'm opening this ticket as a reminder to address this following the coming PDL-2.015 release. As always, feedback and suggestions on bug fixes are welcome.

https://sourceforge.net/p/pdl/bugs/411/

@mohawk2
Copy link
Member Author

mohawk2 commented Apr 15, 2022

To capture on here @drzowie's expansion on this:

The original bug happens because of the interaction between reshape()'s non-canonical / legacy inplace operation, and the way mv() establishes a trans to relate dimensions. mv() establishes a trans() structure that relates a certain dimension (the n1 field in the trans) in the parent to the corresponding other dimension (the n2 field in the trans). The dimlists implicitly have the same length. If reshape() is carried out on either of the PDLs in the mv() relationship, then it is possible to shorten the dimlist of one so that it the mv() relationship is no longer valid.

The real problem isn't with using reshape() as a shorthand for a sequence of splitdim operations, it's with using reshape to modify a PDL that is itself an endpoint of a particular trans.

It's possible to do that with sever() -- e.g.

$a = pdl([0,1],[2,3]);
$b = $a->mv(-1,0)->sever->reshape(4);

Another convenient way to implement reshape-as-splitdim sequence cleanly would be to write a new method (maybe called newshape?) that produces a new PDL and links it to the parent with a new kind of trans structure, rather than (as reshape does) modifying the parent in-place.

And:

To be even more clear, the child of an mv operation doesn't actually have its own dimlist or data pointer: the trans code mocks up the dimlist pointer on-the-fly based on the dimlist of the parent. So modifying the mocked-up dimlist in-place with reshape/setdims is a very bad idea (which is why it corrupts the parent PDL's data and/or causes segfaults).

@mohawk2
Copy link
Member Author

mohawk2 commented Apr 29, 2022

A / the solution to this problem would be to make a new withdims operation, which would be a sort of expanded mv but just replacing the dims list entirely. It would zero-pad if the new "shape" were larger than the old one. It would functionally replace reshape and setdims (at least).

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

No branches or pull requests

1 participant