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

Add keepdims keyword argument #701

Merged
merged 5 commits into from
Jan 15, 2025
Merged

Add keepdims keyword argument #701

merged 5 commits into from
Jan 15, 2025

Conversation

v923z
Copy link
Owner

@v923z v923z commented Dec 30, 2024

Add requested keyword argument keepdims to some numerical functions

@v923z
Copy link
Owner Author

v923z commented Dec 30, 2024

@kwagyeman I think this should address the issue that you raised. I'll re-work this in the coming days, for the simple reason that others have requested the out keyword argument, but that's not going to change anything with respect to keepdims. In other words, you won't even notice that, except, perhaps for the code size.

@kwagyeman
Copy link

Awesome! Once this is merged we can integrate it into OpenMV.

@v923z
Copy link
Owner Author

v923z commented Dec 30, 2024

Awesome! Once this is merged we can integrate it into OpenMV.

If this is urgent for you, I can merge this right away. I really see no reason for waiting otherwise.

@kwagyeman
Copy link

It's not urgent. Please test and build whatever confidence you need in the PR along with adding the other feature.

@kwagyeman
Copy link

@v923z - Any updates? Back to work now and would like to get this merged.

@v923z
Copy link
Owner Author

v923z commented Jan 14, 2025

I can merge the current implementation right away. I have the skeleton of the improved version, but I can't promise at the moment that I can flesh out all detail this weekend. So, if this is urgent, I would propose that we merge what we have now, and future improvements won't affect the user, for the interface will remain the same.

@v923z
Copy link
Owner Author

v923z commented Jan 14, 2025

I would emphasize that this is a correctly working implementation.

@kwagyeman
Copy link

That sounds good. That would unblock me after you release what you have.

@v923z v923z merged commit 903506c into master Jan 15, 2025
16 of 32 checks passed
@v923z v923z deleted the keepdims branch January 15, 2025 09:29
@v923z
Copy link
Owner Author

v923z commented Jan 15, 2025

@kwagyeman I merged this before realising that I hadn't yet updated the documentation. Can you do the checkout against the commit, or do you need a release tag? I'll try to fix the docs tonight, though, I'm a bit booked out for today. I would release this only with the correct documentation.

@kwagyeman
Copy link

I can wait for that to be updated.

@v923z
Copy link
Owner Author

v923z commented Jan 15, 2025

OK. I can definitely fix that over the weekend.

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.

2 participants