Skip to content

Conversation

@sergey-yaroslavtsev
Copy link
Collaborator

@sergey-yaroslavtsev sergey-yaroslavtsev commented Dec 22, 2025

Closes #1162
Following suggestions from comments in the issue to switch all .shap= to either reshape or ravel.

New pattern using ndarray.reshape
arr = numpy.zeros((1,3,4));arr = arr.reshape(-1, 6);print(arr.shape)

New pattern using numpy.ravel
arr = numpy.array(0);arr=numpy.ravel(arr);print(arr.shape)

Fixed places are less than mentioned "485". This ".shape=" remained in .c files and in some comments, where for my understanding they should not be changed. Or am I wrong?
In .pyx files it was changed.

 pymca --test

runs OK.

Please have a brief look.

Note:
in many cases I used "*" to unpack the input shape but it is not required - now I doubt if it makes things clearer or worse.

b = numpy.array([1, 2, 3, 4, 5, 6, 7, 8])
b = b.reshape((2,4))
b
array([[1, 2, 3, 4],
       [5, 6, 7, 8]])
b = numpy.array([1, 2, 3, 4, 5, 6, 7, 8])
b = b.reshape([2, 4])
b
array([[1, 2, 3, 4],
       [5, 6, 7, 8]])
b = numpy.array([1, 2, 3, 4, 5, 6, 7, 8])
b = b.reshape(*(2,4))
b
array([[1, 2, 3, 4],
       [5, 6, 7, 8]])

Copy link
Collaborator

@woutdenolf woutdenolf left a comment

Choose a reason for hiding this comment

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

LGTM.

For reference:

  • Unlike numpy.reshape the instance method ndarray.reshape can take either a tuple but also an unpacked tuple. This PR mixes both conventions. I don't mind.
  • reshape and ravel only copies the array when needed. In contrast flatten (which we do not use in this PR) always copies.

As for the cython generated code, I think we should do it there as well (not manually). But that can be another PR.

For example this will break in numpy 2.4.0 because to the strides trick:

} else {__pyx_pybuffernd_mask.diminfo[0].strides = __pyx_pybuffernd_mask.rcbuffer->pybuffer.strides[0]; __pyx_pybuffernd_mask.diminfo[0].shape = __pyx_pybuffernd_mask.rcbuffer->pybuffer.shape[0];

See for example silx-kit/pyFAI#2711.

These .c files say /* Generated by Cython 0.29.32 */. Not sure what's going on here. Why are they in the repo @vasole ?

Edit: it seems like setup.py anticipates cython not being available in which case it falls back to the c files. We installed cython and re-generated the .c files but no changes were applied, even when numpy 2.4.0 was installed.

numpy.sum(tmpData, 2),
self._stackImageData[i:i+step,:])
tmpData.shape = step*shape[1], shape[2]
tmpData = tmpData.reshape(step*shape[1], shape[2])
Copy link
Collaborator

@woutdenolf woutdenolf Dec 23, 2025

Choose a reason for hiding this comment

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

Ok I did not know that elements of the shape parameter can be passed in as separate arguments.

https://numpy.org/devdocs/reference/generated/numpy.ndarray.reshape.html

FYI The free numpy.reshape function only accepts a tuple.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

but you did it in the suggestion for issue #1162

New pattern using ndarray.reshape
arr = numpy.zeros((1,3,4));arr = arr.reshape(-1, 6);print(arr.shape)
(2, 6)

and to be concistent between
arr.reshape(X, Y) + arr.reshape(*tuple)
or
arr.reshape((X, Y)) + arr.reshape(tuple)
I chose 1st option

Copy link
Collaborator

@woutdenolf woutdenolf Dec 23, 2025

Choose a reason for hiding this comment

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

Indeed. I just didn't know before. I've only used the free numpy.reshape until I looked at the easiest way to refactor setting shape.

else:
ddict['result'][label] = ymatrix
ddict['result'][label].shape = (len(ddict['result'][label]),)
ddict['result'][label] = numpy.ravel(ddict['result'][label])
Copy link
Collaborator

Choose a reason for hiding this comment

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

That's indeed another case where we can use numpy.ravel.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@sergey-yaroslavtsev We introduced silent flattening here. See #1170 (comment).

if not gotIt:
raise ValueError("Unmatched dimensions following C order")
data.shape = xsize, oldDataShape[i+1:]
data = data.reshape(xsize, *oldDataShape[i+1:])
Copy link
Collaborator

Choose a reason for hiding this comment

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

Good catch. This was a bug.

ncols = values[2]
self.nSpectra = nrows * ncols
data.shape = [len(data)/3, 3]
data = data.reshape(int(len(data)/3), 3)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Good catch

else:
spectra = data[badMask, iXMin:iXMax+1]
spectra.shape = badMask.sum(), -1
spectra = spectra.reshape(int(badMask.sum()), -1)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Good catch.

@vasole vasole merged commit db58840 into master Dec 23, 2025
6 checks passed
@vasole
Copy link
Member

vasole commented Dec 23, 2025

Thank you!

@vasole vasole deleted the numpy_shape branch December 23, 2025 18:26
Copy link
Collaborator

@woutdenolf woutdenolf left a comment

Choose a reason for hiding this comment

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

I propose the fix the three silent flattenings we introduced here. See #1170 (comment) for the preferred way.

else:
ddict['result'][label] = ymatrix
ddict['result'][label].shape = (len(ddict['result'][label]),)
ddict['result'][label] = numpy.ravel(ddict['result'][label])
Copy link
Collaborator

Choose a reason for hiding this comment

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

@sergey-yaroslavtsev We introduced silent flattening here. See #1170 (comment).

def diago(self, k, m):
mat = numpy.zeros([m,m], numpy.float64)
mat.shape=[m*m]
mat = numpy.ravel(mat)
Copy link
Collaborator

Choose a reason for hiding this comment

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

@sergey-yaroslavtsev We introduced silent flattening here. See #1170 (comment).

Comment on lines -1944 to +1945
xdata.shape= [len(xdata),]
ydata.shape= [len(ydata),]
xdata = numpy.ravel(xdata)
ydata = numpy.ravel(ydata)
Copy link
Collaborator

Choose a reason for hiding this comment

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

@sergey-yaroslavtsev We introduced silent flattening here. See #1170 (comment).

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.

Heads up about possible setting shape without using reshape deprecation

4 participants