Skip to content

[opt_transport] Replace np.sum(a * b) with np.vdot(a, b)#475

Merged
mmcky merged 2 commits intoQuantEcon:mainfrom
suda-yuga:suda-yuga-patch-4
Jul 21, 2025
Merged

[opt_transport] Replace np.sum(a * b) with np.vdot(a, b)#475
mmcky merged 2 commits intoQuantEcon:mainfrom
suda-yuga:suda-yuga-patch-4

Conversation

@suda-yuga
Copy link
Contributor

Replace np.sum(a * b) with np.vdot(a, b)
#463

@mmcky mmcky requested a review from HumphreyYang July 1, 2025 02:15
@HumphreyYang
Copy link
Member

Hi @suda-yuga,

Thanks so much for your PR! This is a nice application of vdot. I am curious would np.sum(a*b) be more transparent than np.vdot(a, b) in this case as we are summing over a 2-D matrix?

@suda-yuga
Copy link
Contributor Author

Hi @suda-yuga,

Thanks so much for your PR! This is a nice application of vdot. I am curious would np.sum(a*b) be more transparent than np.vdot(a, b) in this case as we are summing over a 2-D matrix?

Thank you for your comment.

You're absolutely right — in this case, np.sum(a * b) is arguably more transparent and easier to read when working with 2-D matrices.

The motivation behind this change was based on prior discussions (#463) suggesting that a @ b or np.vdot(a, b) can be more efficient than np.sum(a * b) due to avoiding intermediate arrays and leveraging BLAS-level optimizations. I applied the same reasoning to the Frobenius inner product of matrices here.

That said, I agree with you — clarity matters, and np.sum(a * b) may better communicate the intent in this specific context. I'm happy to revert or revise the change depending on what's preferred for readability.

@HumphreyYang
Copy link
Member

HumphreyYang commented Jul 21, 2025

Thanks @suda-yuga! I'm happy either way. I just feel that it's less obvious to readers what we're trying to compute here, unlike the more straightforward cases where we have two 1-D arrays. For the sake of presentation, I'm inclined to close this PR—though I really like what you've done!

Hi @mmcky, I will leave it to you to make the call!

@oyamad
Copy link
Member

oyamad commented Jul 21, 2025

Maybe write something like "Here we use np.vdot (with a link to https://numpy.org/doc/stable/reference/generated/numpy.vdot.html) (instead of np.dot or @), where X and C are 2-D arrays"?

@oyamad
Copy link
Member

oyamad commented Jul 21, 2025

Or "Here we use np.vdot for the trace inner product of X and C".
(The notation "tr (C' X)" is already introduced (with no explanation).)

@mmcky
Copy link
Contributor

mmcky commented Jul 21, 2025

Thanks @oyamad for the suggestion.

I think being explicit around the use of vdot is a good idea, specifying what it achieves. I have added your second suggestion with a link to the vdot documentation.

@mmcky
Copy link
Contributor

mmcky commented Jul 21, 2025

thanks @HumphreyYang and @oyamad for your valuable feedback and review.

Thanks @suda-yuga for the PR.

@mmcky mmcky merged commit 1ca080e into QuantEcon:main Jul 21, 2025
6 checks passed
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.

4 participants