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

BUG: linalg.polar: empty array support #20432

Closed
wants to merge 0 commits into from
Closed

Conversation

redha2404
Copy link

Reference issue

Check the box "polar" in #20372

What does this implement/fix?

If a is empty, the polar function now returns p and u of the right types. Before, an error occurred on empty array.

Additional information

It is just a first PR for #20372 to see I understand the problem

@lucascolley lucascolley requested review from ev-br and removed request for ilayn and larsoner April 9, 2024 22:50
@lucascolley lucascolley changed the title Fixing problem with polar for empty parameters BUG: linalg.polar: empty array support Apr 9, 2024
@lucascolley lucascolley added the defect A clear bug or issue that prevents SciPy from being installed or used as expected label Apr 9, 2024
@lucascolley
Copy link
Member

thanks @redha2404 , can you add a regression test? Examples can be found in the test file changes in https://github.com/scipy/scipy/pull/20295/files.

Copy link
Member

@lucascolley lucascolley left a comment

Choose a reason for hiding this comment

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

Looks reasonable to me, thanks! I'll let @ev-br take a look. CI failure is unrelated.

scipy/linalg/tests/test_decomp_polar.py Outdated Show resolved Hide resolved
Copy link
Member

@ev-br ev-br left a comment

Choose a reason for hiding this comment

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

This is a good start but needs a tweak to properly account for integer inputs.

scipy/linalg/_decomp_polar.py Outdated Show resolved Hide resolved
scipy/linalg/_decomp_polar.py Outdated Show resolved Hide resolved
scipy/linalg/tests/test_decomp_polar.py Outdated Show resolved Hide resolved
@redha2404
Copy link
Author

redha2404 commented Apr 14, 2024

I think the problem with int is now resolved

@ev-br
Copy link
Member

ev-br commented Apr 14, 2024

Thanks. The dtype is indeed correct now.
However I've a follow-up concern, sorry for not thinking about it before.
Let's see:

In [44]: a
Out[44]: array([], shape=(0, 3), dtype=float32)

In [45]: u, p = polar(a)

In [46]: p
Out[46]: 
array([[ 8.6914245e-20,  3.0839777e-41,  0.0000000e+00],
       [ 0.0000000e+00,  2.8025969e-45,  0.0000000e+00],
       [           nan,            nan, -6.4903711e+32]], dtype=float32)

In [47]: u @ p
Out[47]: array([], shape=(0, 3), dtype=float32)

The dtype here is irrelevant, but the contents of p is. It is documented to be positive semidefinite, but what is returned clearly is not. There is a hint in the implementation: you use np.empty_like(a.T @ a). Can you explain why you used empty instead of e.g. a.T @ a itself?

@redha2404
Copy link
Author

redha2404 commented Apr 14, 2024

I didn't notice np.empty_like puts such values. I wanted to precise the dtype I want on p but I just noticed that a.T @ a does the conversion int_array -> (float64, float64)

>>> u = np.empty((0,3), dtype=int)
>>> p = a.T @ a
>>> p.dtype
dtype('float64')

So, should I just put e.g, u = np.copy(a) and p=a.T @ a for 'right' ?

@redha2404
Copy link
Author

Oh, not for u. It doesn't transform int to float. I should not change the line for u

@ev-br ev-br added the needs-work Items that are pending response from the author label Apr 16, 2024
@redha2404
Copy link
Author

I changed it as you suggested it

@melissawm
Copy link
Contributor

Hi @redha2404 - did you mean to close this?

@ev-br
Copy link
Member

ev-br commented Apr 18, 2024

I wanted to give it the last look-though before pressing the green button but the patch is now gone.

@redha2404
Copy link
Author

I wanted to change the branch from which I merge because it was from my main branch. Should I make another PR with the same modifications ?

@lucascolley
Copy link
Member

I wanted to change the branch from which I merge because it was from my main branch. Should I make another PR with the same modifications ?

Yeah, sounds good 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
defect A clear bug or issue that prevents SciPy from being installed or used as expected needs-work Items that are pending response from the author scipy.linalg
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants