Sanity check parent of Vector_numpy_integer_dense

sagemath / sage

Main repository of SageMath

https://www.sagemath.org

Other

1.47k stars 485 forks source link

Sanity check parent of Vector_numpy_integer_dense #38938

Closed user202729 closed 1 week ago

user202729 commented 2 weeks ago

As in the title.

This appears to be what the author intend it to be, previously it's possible to construct with base_ring not ZZ but the elements being returned are ZZ anyway. (plus self._sage_dtype = ZZ.)

:memo: Checklist

[x] The title is concise and informative.
[x] The description explains in detail what this PR is about.
[ ] I have linked a relevant issue or discussion.
[x] I have created tests covering the changes.
[ ] I have updated the documentation and checked the documentation preview.

github-actions[bot] commented 2 weeks ago

Documentation preview for this PR (built with commit 43b8c003afb2d5c924725cda3107c82a06089ba5; changes) is ready! :tada: This preview will update shortly after each push to this PR.

user202729 commented 2 weeks ago

But… the class is never called from anywhere else? (as far as I can see)

This is unlike Vector_real_double_dense where it is automatically selected by def vector(): when the base ring is RR.

If not called by the user what is it for?

the class was first created in 5b288c35d91cbbf1aaca136cfa6cb5a35b5bbe1f

tscrim commented 1 week ago

I can only guess, but probably for eventually being able to set the backend of FreeModule. That shouldn't be too hard to do, but it probably would take a bit of time and effort to do. As-is, this feels somewhat like lip service to people who want numpy stuff...

user202729 commented 1 week ago

The problem is using that class is inherently dangerous because it has no overflow check… right (?)

tscrim commented 1 week ago

Yes, there are a number of issues as I don't think Matthias fully implemented his idea. Well, perhaps we will just include this for now as a way to make this a little safer to use, which we can later remove if the invocation becomes less user-facing (not that it is very much so right now anyways...). Thoughts?

tscrim commented 1 week ago

PS - I should also say the only thing I can do to contribute to implementing such a backend right now is in reviewing code.

user202729 commented 1 week ago

perhaps we will just include this for now as a way to make this a little safer to use, which we can later remove if the invocation becomes less user-facing

Sounds good to me.

Another alternative I can see is to explicitly document the limitations. (The original motivation for this PR is that I see the code there, but was confused what the class is used for, so I try to is instantiate it with Zmod(p) and it actually works except that it produces erratic behavior.)

I think that would be sufficient, because after all, users would never encounter this issue themselves unless they actually come across the corresponding page in the documentation.