Fix remain arr.shape=

[sergey-yaroslavtsev]

Related to #1165 and #1162
Fixes remain arr.shape=.
Moreover probably it was a 'bug' since continuum was reshaped by ymatrix. If not please please let me know.

[sergey-yaroslavtsev]

They had double space before = that is probably why i missed them in original fix.

[sergey-yaroslavtsev]

I found these remains accidentally, while investigating *.c files for same reason. At the end I have filling that there is nothing to be done for them for now. Because direct rebuild of *.c files using

python setup.py build_ext --inplace

do not change them. So probably .shape= there have different meaning and not related to numpy...
But maybe I am too newbie in this field.

[vasole]

Moreover probably it was a 'bug' since continuum was reshaped by ymatrix. If not please please let me know.

No, it was not a bug. Continuum must have the same length as ymatrix.

[vasole]

Personally I prefer to use reshape rather than ravel to simplify tracking of changes, but it is not a big deal.

My recommendation would be to use ravel as replacement of arr.shape = -1 and arr.reshape for all the other cases.

[sergey-yaroslavtsev]

Personally I prefer to use reshape rather than ravel to simplify tracking of changes, but it is not a big deal.

My recommendation would be to use ravel as replacement of arr.shape = -1 and arr.reshape for all the other cases.

In #1165 it was already done as here. We can change if needed but it seems to be same things.

[vasole]

In #1165 it was already done as here. We can change if needed but it seems to be same things.

Yes, I think there was only one case of use of ravel to replace shape = len... and all the others were associated to shape = -1, but I do not mind. It is just that using ravel at this precise point is not the exact behavior of previous code where there was an implicit assert of continuum and ymatrix having the same length.

[vasole]

Now you have a bug. You have made continuum equal to ymatrix. Only the shape is equal not the content!

[vasole]

Now you have a bug.

x.reshape(len(y)) is not the same as numpy.ravel(y)

[woutdenolf]

in this case there is a clear difference between using reshape

ddict['result']['ymatrix'] = ddict['result']['ymatrix'].reshape(len(ddict['result']['ymatrix']))
ddict['result']['continuum'] = ddict['result']['continuum'].reshape(len(ddict['result']['ymatrix']))

and using ravel

ddict['result']['ymatrix'] = numpy.ravel(ddict['result']['ymatrix'])
ddict['result']['continuum'] = numpy.ravel(ddict['result']['continuum'])

The key difference is how they behave for unexpected shapes.

numpy.ravel will always return a 1-D array, even if the input is (N, M) or otherwise multi-dimensional, which can silently mask bugs.

The reshape(len(...)) approach assumes the input is effectively 1-D (e.g. (N,), (N, 1), (N, 1, 1)). If that assumption is violated, NumPy will raise an exception. In this case, that’s preferable, because flattening a truly 2-D array would be unexpected and semantically incorrect.

Additionally, reshaping continuum using len(ddict['result']['ymatrix']) implicitly enforces that both arrays have compatible lengths. If they don’t, this will also raise, which helps catch shape mismatches early.

For these reasons, I’d prefer the explicit reshape version so we fail fast on invalid or inconsistent shapes rather than silently flattening them.

Note: in #1165 we did replace arr.shape = -1 with numpy.ravel because both silently flatten. Actually we did introduce three cases of silent flattening which was not silent before. To be fixed (see post-merge review).

[sergey-yaroslavtsev]

Concerning the #1165 places which you've mentioned and probably some others (where ravel(arr(...) was used) should be then fixed to reshape(len(...)) by the same logic for easier bug catch in future.

Do you mind if I fix it in this PR?

[sergey-yaroslavtsev]

Now you have a bug. You have made continuum equal to ymatrix. Only the shape is equal not the content!

Yes indeed, sorry.

[sergey-yaroslavtsev]

Concerning the #1165 places which you've mentioned and probably some others (where > ravel(arr(...) was used) should be then fixed to reshape(len(...)) by the same logic for easier bug catch in future.

I did not found other places. All other ravel were used to substitute .shape=-1

Personally I prefer to use reshape rather than ravel to simplify tracking of changes, but it is not a big deal.

My recommendation would be to use ravel as replacement of arr.shape = -1 and arr.reshape for all the other cases.

Now it is like this.