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

Fast conversion from numpy ndarray to vector #38834

Open
wants to merge 12 commits into
base: develop
Choose a base branch
from

Conversation

user202729
Copy link
Contributor

@user202729 user202729 commented Oct 21, 2024

As explained, this allows fast conversion from numpy ndarray to some types of vector.

Side note: is cimport numpy as np safe? what happens if Sage is built without numpy?


Extra context: I originally tried to use memory view, but for some reason it doesn't work: cython/cython#6442

Traceback (most recent call last):
  File "/sage/src/bin/sage-ipython", line 10, in <module>
    banner()
  File "/sage/src/sage/misc/banner.py", line 114, in banner
    print(banner_text(full=True))
          ^^^^^^^^^^^^^^^^^^^^^^
  File "/sage/src/sage/misc/banner.py", line 69, in banner_text
    import sage.all
  File "/sage/src/sage/all.py", line 91, in <module>
    from sage.algebras.all   import *
  File "/sage/src/sage/algebras/all.py", line 25, in <module>
    from sage.algebras.quatalg.all import *
  File "/sage/src/sage/algebras/quatalg/all.py", line 1, in <module>
    from sage.algebras.quatalg.quaternion_algebra import QuaternionAlgebra
  File "/sage/src/sage/algebras/quatalg/quaternion_algebra.py", line 76, in <module>
    from . import quaternion_algebra_cython
  File "sage/algebras/quatalg/quaternion_algebra_cython.pyx", line 1, in init sage.algebras.quatalg.quaternion_algebra_cython
  File "sage/matrix/matrix_integer_dense.pyx", line 1, in init sage.matrix.matrix_integer_dense
  File "sage/modules/vector_integer_dense.pyx", line 1, in init sage.modules.vector_integer_dense
AttributeError: type object 'sage.modules.vector_integer_dense.array' has no attribute '__getattr__'. Did you mean: '__setattr__'?

Using the old buffer syntax is a bit slower.

📝 Checklist

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

@user202729
Copy link
Contributor Author

Now use old buffer syntax, it should work.

Similar changes could be applied to vector_modn_dense.pyx and vector_integer_dense.pyx as well…

Copy link

github-actions bot commented Oct 22, 2024

Documentation preview for this PR (built with commit 7c54e21; changes) is ready! 🎉
This preview will update shortly after each push to this PR.

@user202729
Copy link
Contributor Author

What am I supposed to do with the test? Move all numpy-related functions to a separate file and lazily import it? (but then won't the construction of any vector import numpy anyway?)

Copy link
Collaborator

@tscrim tscrim left a comment

Choose a reason for hiding this comment

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

I believe this Cython module gets imported at startup, which since you now have a global cimport of numpy causes the test to fail. Indeed, if I add a cimport numpy to integer.pyx (which is 100% imported at startup), then that test fails. So either you need to make everything local to that method or make sure this file is not loaded at startup.

@user202729
Copy link
Contributor Author

user202729 commented Nov 8, 2024

Lazy import appears to be difficult. I end up with a hacky solution, where I make the function in Python, and cast the pointer to uintptr_t.

I tried to use cdef extern but it doesn't seem to work (I don't know how to specify the correct library to load).


Annoyingly enough there doesn't seem to be mzd_write_bits to write multiple bits in a row, but mzd_set_ui and mzd_xor_bits should work.

It may be interesting to test if precompute the word and use the combination to set the whole word in at once would be faster. (Maybe it will, if for nothing other than loop unrolling.)

Copy link
Collaborator

@tscrim tscrim left a comment

Choose a reason for hiding this comment

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

Thank you. This is okay for me.

For your optimizations, that is beyond my technical abilities and knowledge. Although adding such functions would need to be done upstream I believe.

@user202729
Copy link
Contributor Author

user202729 commented Nov 8, 2024

In https://github.com/sagemath/sage/actions/runs/11736415975/job/32695629971, it gives ModuleNotFoundError: No module named 'sage.modules.numpy_util'

Maybe the test failure is caused by the pxd file not exist… not sure, it works locally for me. (After all we're importing from Python instead of from C, why is the pxd needed?)

I'll just push the commit to test it out, don't merge yet (until tests pass).

@user202729
Copy link
Contributor Author

Should be done now.

Test failures are:

2024-11-08T14:13:41.9618970Z sage -t --warn-long 5.0 --random-seed=103363666288970081125621785486426094449 src/sage/categories/simplicial_sets.py  # 1 doctest failed
2024-11-08T14:13:41.9623870Z sage -t --warn-long 5.0 --random-seed=103363666288970081125621785486426094449 src/sage/rings/number_field/galois_group.py  # 1 doctest failed [failed in baseline: unreported failure on macOS (sort order) in GaloisGroup_v2.artin_symbol, https://github.com/sagemath/sage/actions/runs/9525536510/job/26259809272?pr=37998]
2024-11-08T14:13:41.9628610Z sage -t --warn-long 5.0 --random-seed=103363666288970081125621785486426094449 src/sage/rings/number_field/number_field.py  # 2 doctests failed [failed in baseline: unreported failure on macOS (sort order) in NumberField_absolute.[optimized_]subfields, https://github.com/sagemath/sage/actions/runs/9525536510/job/26259809272?pr=37998]
2024-11-08T14:13:41.9635330Z sage -t --warn-long 5.0 --random-seed=103363666288970081125621785486426094449 src/sage/rings/qqbar.py  # 1 doctest failed [failed in baseline: unreported failure on macOS seen in https://github.com/sagemath/sage/actions/runs/9525536510/job/26259809272?pr=37998]

First one #38940 , the rest are clearly written as unrelated.

Copy link
Collaborator

@tscrim tscrim left a comment

Choose a reason for hiding this comment

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

I feel like there should also be a .pxd file, right? Well, I guess that is not strictly necessary given the setup. I forget exactly when they are needed.

@user202729
Copy link
Contributor Author

If I understood correctly you need it when you want cimport from it to work… in this case we don't actually need that (only need Python import to work).

I can add for consistency though. (though it won't be tested so is also dangerous) The alternative is to leave documentation in the pyx explaining what's going on with the pxd being missing and to add it when useful.

@tscrim
Copy link
Collaborator

tscrim commented Nov 15, 2024

Let's add it for consistency and in case someone down the road has a use case for it. Thank you.

Copy link
Collaborator

@tscrim tscrim left a comment

Choose a reason for hiding this comment

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

Thank you. LGTM.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants