Skip to content

MAINT: logsumexp: properly handle complex sign #19870

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 7 commits into from
Jan 16, 2024
Merged

MAINT: logsumexp: properly handle complex sign #19870

merged 7 commits into from
Jan 16, 2024

Conversation

jakevdp
Copy link
Member

@jakevdp jakevdp commented Jan 12, 2024 •
edited
Loading

Fixes #19854

I opted to have logsumexp return a sign that matches the convention of np.sign for whichever numpy version is installed.

@jakevdp jakevdp mentioned this pull request Jan 12, 2024
Closed
18 tasks
@jakevdp jakevdp force-pushed the logsumexp-sign branch 3 times, most recently from dd5a305 to f885d1f Compare January 12, 2024 01:41
@lucascolley lucascolley added the maintenance Items related to regular maintenance tasks label Jan 12, 2024
@rgommers rgommers changed the title logsumexp: properly handle complex sign MAINT: logsumexp: properly handle complex sign Jan 12, 2024
@rgommers
Copy link
Member

Thanks @jakevdp! Seems like a reasonable choice to me. I haven't thought about it as hard as you and @mhvk though. @mhvk would you mind having a look?

@rgommers rgommers added this to the 1.13.0 milestone Jan 12, 2024
mhvk
mhvk approved these changes Jan 12, 2024
View reviewed changes
Copy link

@mhvk mhvk left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Great! In-line a comment about whether we should just use the "correct" sign all the time, but since what you have gives exactly the same answer, either route is fine.

p.s. Like your numpy-version-agnostic test!

@jakevdp
Copy link
Member Author

jakevdp commented Jan 12, 2024

Updated based on discussion in #19854 (comment)

mdhaber
mdhaber reviewed Jan 12, 2024
View reviewed changes
mhvk
mhvk reviewed Jan 12, 2024
View reviewed changes
Copy link

@mhvk mhvk left a comment

Choose a reason for hiding this comment

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

looking at this more, one wonders whether this function really makes sense for complex, but some comments in-line about it. Only first one about a_max is important.

mhvk
mhvk reviewed Jan 16, 2024
View reviewed changes
Copy link

@mhvk mhvk left a comment

Choose a reason for hiding this comment

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

Here's one more suggested change (I had it as a pending comment for about a week it seems...)

rgommers
rgommers approved these changes Jan 16, 2024
View reviewed changes
Copy link
Member

@rgommers rgommers left a comment

Choose a reason for hiding this comment

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

LGTM, let's give this a go - thanks @jakevdp and @mhvk!

@rgommers rgommers merged commit a403bde into scipy:main Jan 16, 2024
@jakevdp jakevdp deleted the logsumexp-sign branch January 16, 2024 21:21
@jakevdp jakevdp mentioned this pull request Jan 16, 2024
Merged
@EarlMilktea EarlMilktea mentioned this pull request Sep 25, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mdhaber mdhaber mdhaber left review comments

@rgommers rgommers rgommers approved these changes

@person142 person142 Awaiting requested review from person142 person142 is a code owner

@steppi steppi Awaiting requested review from steppi steppi is a code owner

+1 more reviewer

@mhvk mhvk mhvk approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
maintenance Items related to regular maintenance tasks scipy.special
Projects
None yet
Milestone
1.13.0
Development

Successfully merging this pull request may close these issues.

scipy.special.logsumexp for complex input with return_sign=True under numpy 2.0
5 participants
@jakevdp @rgommers @mhvk @mdhaber @lucascolley