Opened 2 years ago

Closed 11 months ago

Last modified 11 months ago

#25366 closed enhancement (fixed)

Expose the function intervalproducts from Harvey's hypellfrob

Reported by: alexjbest Owned by:
Priority: major Milestone: sage-8.9
Component: c_lib Keywords: sd87
Cc: edgarcosta, jdemeyer Merged in:
Authors: Alex J. Best Reviewers: Frédéric Chapoton
Report Upstream: N/A Work issues:
Branch: 89e8fd4 (Commits) Commit: 89e8fd420f58ec3f5379e2a4c824803959a04590
Dependencies: Stopgaps:

Description

The function intervalproducts in hypellfrob will be useful for other similar applications. This ticket wraps that function for the rest of sage, it is necessary to have some basic functionality for ntl's mat_ZZ_p for this.

Change History (41)

comment:1 Changed 2 years ago by alexjbest

  • Branch set to u/alexjbest/interval-products

comment:2 Changed 2 years ago by alexjbest

  • Commit set to 96e07833778c9d050b53c79b56ba903ff350d514
  • Status changed from new to needs_review

New commits:

3e1041dwrap ntl_mat_ZZ_p functionality
96e0783wrap harvey's intervalproducts

comment:3 Changed 2 years ago by alexjbest

  • Branch changed from u/alexjbest/interval-products to u/alexjbest/interval-products-no-wrap
  • Commit changed from 96e07833778c9d050b53c79b56ba903ff350d514 to 6bbe6c01c6bcf56fe05aa7119444285de47a6d64
  • Dependencies #25365 deleted
  • Status changed from needs_review to needs_work

New version of the code which does not add a lot of extra wrappers, still WIP (style is horrible) but the code works so review of that would help.


New commits:

6eff2a7testing some
6bbe6c0🎶 I can call my code with no wrapping class, no wrapping class, no wrapping class 🎵

comment:4 follow-ups: Changed 2 years ago by jdemeyer

Do you really need this hack?

ZZ_p_c (*get "operator()") (long i, long j)

Cython should understand operator() just fine.

comment:5 in reply to: ↑ 4 ; follow-up: Changed 2 years ago by alexjbest

Replying to jdemeyer:

Do you really need this hack?

ZZ_p_c (*get "operator()") (long i, long j)

Cython should understand operator() just fine.

I should say I am a cython noob, so the answer to all such questions is going to be no! Thanks for telling me, I just copy what is there already for the most part to get something working. I'll try and get rid of it.

comment:6 in reply to: ↑ 5 ; follow-up: Changed 2 years ago by jdemeyer

Replying to alexjbest:

I just copy what is there already

The NTL interface is probably the worst Cython code in Sage, so that's not a good recommendation.

comment:7 Changed 2 years ago by jdemeyer

Even this is dubious:

cdef cppclass mat_ZZ_p_c "mat_ZZ_p"

I would just keep the mat_ZZ_p name, no need to append _c (and if you do change the name, it might be to make it clear that it comes from NTL, so NTL_mat_ZZ_p would make more sense).

Last edited 2 years ago by jdemeyer (previous) (diff)

comment:8 in reply to: ↑ 6 ; follow-up: Changed 2 years ago by alexjbest

Replying to jdemeyer:

Replying to alexjbest:

I just copy what is there already

The NTL interface is probably the worst Cython code in Sage, so that's not a good recommendation.

Sure I've definitely got that impression, looking at the linbox stuff in comparison say. But as I need ntl functionality I have to either dedicate serious time to understanding cython and fixing everything, or just add new code that is hopefully no worse than the existing, and somewhat consistent with the existing at that. I do appreciate the comments though.

comment:9 follow-up: Changed 2 years ago by jdemeyer

Avoid the iteration syntax 0 <= k < numintervals, Cython understands range().

The existing NTL code also has a tendency to over-use pointers. For example, in interval_products I see no reason for

cdef mat_ZZ_p_c * out = new mat_ZZ_p_c()

You might as well use

cdef mat_ZZ_p_c out

and replace out[0] by out in the rest of the code (and remove del out). The same for mm0 and mm1: new_ntl_matrix_modn_dense doesn't need to allocate a new matrix, it might as well fill in an existing matrix.

comment:10 Changed 2 years ago by git

  • Commit changed from 6bbe6c01c6bcf56fe05aa7119444285de47a6d64 to be6948f676a22dd4ab62287ff57306799066b4da

Branch pushed to git repo; I updated commit sha1. New commits:

be6948foperator -> get

comment:11 in reply to: ↑ 4 ; follow-up: Changed 2 years ago by alexjbest

Replying to jdemeyer:

Do you really need this hack?

ZZ_p_c (*get "operator()") (long i, long j)

Cython should understand operator() just fine.

Without it, it didn't seem to _just work_ as I expected i.e. calling out(i,j) C++ style, I have replaced it with the (less horrific)

        ZZ_p_c get(long i, long j)

though.


New commits:

be6948foperator -> get
Last edited 2 years ago by alexjbest (previous) (diff)

comment:12 in reply to: ↑ 8 Changed 2 years ago by jdemeyer

Replying to alexjbest:

But as I need ntl functionality I have to either dedicate serious time to understanding cython

I would argue that the time you spend learning Cython is much better spend than trying to understand the Sage <-> NTL wrappers. I'm not kidding when I said that it's probably the worst Cython code in all of Sage.

comment:13 in reply to: ↑ 11 ; follow-up: Changed 2 years ago by jdemeyer

Replying to alexjbest:

Without it, it didn't seem to _just work_ as I expected i.e. calling out(i,j) C++ style

It should work though. Which error did you get?

comment:14 in reply to: ↑ 9 ; follow-up: Changed 2 years ago by jdemeyer

Replying to jdemeyer:

new_ntl_matrix_modn_dense doesn't need to allocate a new matrix, it might as well fill in an existing matrix.

More precisely, you don't need new_ntl_matrix_modn_dense at all: just move the line A.SetDims(m._nrows, m._ncols) to set_ntl_matrix_modn_dense and call that with an existing matrix like

cdef mat_ZZ_p_c mm

set_ntl_matrix_modn_dense(mm, ...)

comment:15 in reply to: ↑ 13 Changed 2 years ago by alexjbest

Replying to jdemeyer:

Replying to alexjbest:

Without it, it didn't seem to _just work_ as I expected i.e. calling out(i,j) C++ style

It should work though. Which error did you get?

You are right, my mistake, I had the wrong syntax in types.pxd, I suppose I prefer get in this context anyway I guess as its 0 indexed rather than 1 indexed but operator does work.

comment:16 Changed 2 years ago by git

  • Commit changed from be6948f676a22dd4ab62287ff57306799066b4da to c5f70461f6bc2dc78c95f0c808da0cbbd5539299

Branch pushed to git repo; I updated commit sha1. New commits:

c5f7046cleanup based on comments of jdemeyer

comment:17 Changed 2 years ago by git

  • Commit changed from c5f70461f6bc2dc78c95f0c808da0cbbd5539299 to 154ffb1c26269bdef9c44993c1ba8780eb917f33

Branch pushed to git repo; I updated commit sha1. New commits:

154ffb1correctly support matrices which don't fit into linbox float

comment:18 Changed 22 months ago by vdelecroix

  • Milestone changed from sage-8.3 to sage-8.4

update milestone 8.3 -> 8.4

comment:19 in reply to: ↑ 14 Changed 21 months ago by alexjbest

  • Status changed from needs_work to needs_review

Replying to jdemeyer:

Replying to jdemeyer:

new_ntl_matrix_modn_dense doesn't need to allocate a new matrix, it might as well fill in an existing matrix.

More precisely, you don't need new_ntl_matrix_modn_dense at all: just move the line A.SetDims(m._nrows, m._ncols) to set_ntl_matrix_modn_dense and call that with an existing matrix like

cdef mat_ZZ_p_c mm

set_ntl_matrix_modn_dense(mm, ...)

I believe I made all the changes you suggested and the patchbot seems happy, so I'm setting this back to needs review, would appreciate any further thoughts you have.

comment:20 Changed 19 months ago by alexjbest

@jdemeyer any update on this? still seems to merge fine, patchbot still passing!

comment:21 Changed 19 months ago by git

  • Commit changed from 154ffb1c26269bdef9c44993c1ba8780eb917f33 to e0a3749936382b21a0adc456b8933242e69048bc

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

a277dc0testing some
39bc447🎶 I can call my code with no wrapping class, no wrapping class, no wrapping class 🎵
9dd0727operator -> get
147cf8ccleanup based on comments of jdemeyer
0eefa0fcorrectly support matrices which don't fit into linbox float
e0a3749fix doc-pdf

comment:22 Changed 19 months ago by alexjbest

  • Milestone changed from sage-8.4 to sage-8.5

comment:23 Changed 18 months ago by alexjbest

This is now the only thing holding up #20264 so if anyone has any time to look at this again that would be much appreciated!

comment:24 Changed 16 months ago by alexjbest

  • Cc jdemeyer added

comment:25 Changed 16 months ago by alexjbest

  • Milestone changed from sage-8.5 to sage-8.7

comment:26 Changed 16 months ago by jdemeyer

Please follow the Sage coding standards: try to limit lines to less than 80 characters and use 4 spaces for indentation.

comment:27 Changed 16 months ago by jdemeyer

There is a potential overflow in hypellfrob_interval_products_wrapper: you use int where NTL uses long.

comment:28 Changed 16 months ago by jdemeyer

Check you divisions: add from __future__ import division and use // where you want floor division.

comment:29 Changed 16 months ago by jdemeyer

More generally about hypellfrob_interval_products_wrapper: do you really need a wrapper calling a wrapper? Can't you drop one level of indirection by doing it in Cython?

comment:30 Changed 15 months ago by embray

  • Milestone changed from sage-8.7 to sage-8.8

Ticket retargeted after milestone closed (if you don't believe this ticket is appropriate for the Sage 8.8 release please retarget manually)

comment:31 Changed 14 months ago by git

  • Commit changed from e0a3749936382b21a0adc456b8933242e69048bc to dc91a183fbace87e1ad7e684e07e48b351b51adc

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

e28420ftesting some
3ae91aa🎶 I can call my code with no wrapping class, no wrapping class, no wrapping class 🎵
ad065beoperator -> get
d55279fcleanup based on comments of jdemeyer
5e4d7dccorrectly support matrices which don't fit into linbox float
965d5defix doc-pdf
dc91a18pep8-ify

comment:32 Changed 12 months ago by chapoton

  • Branch changed from u/alexjbest/interval-products-no-wrap to public/ticket/25366
  • Commit changed from dc91a183fbace87e1ad7e684e07e48b351b51adc to 54e89297ecfcb1c621f070fb83acefe0c2dc56df

I have made a fresh, rebased and squashed branch.


New commits:

54e8929Expose the function intervalproducts from Harvey's hypellfrob

comment:33 Changed 12 months ago by chapoton

  • Status changed from needs_review to needs_work

This seems to be currently broken.

comment:34 Changed 12 months ago by chapoton

Broken, apparently in 8.7.rc0, maybe due to #27070 ?

comment:35 Changed 12 months ago by git

  • Commit changed from 54e89297ecfcb1c621f070fb83acefe0c2dc56df to 0d3f8f6ea5d15d47d18b56cb30487ceaa795c15a

Branch pushed to git repo; I updated commit sha1. New commits:

b6f1911Merge branch 'public/ticket/25366' of git://trac.sagemath.org/sage into HEAD
0d3f8f6fix bug in ntl call

comment:36 Changed 12 months ago by git

  • Commit changed from 0d3f8f6ea5d15d47d18b56cb30487ceaa795c15a to c0bb7c208b0974ab3582ca8c300707719f8f88f8

Branch pushed to git repo; I updated commit sha1. New commits:

c0bb7c2trac 25366 some details in code and doc

comment:37 Changed 12 months ago by chapoton

  • Reviewers set to Frédéric Chapoton
  • Status changed from needs_work to needs_review

I have changed some little details. If you agree with them, you can set to positive review.

comment:38 Changed 12 months ago by git

  • Commit changed from c0bb7c208b0974ab3582ca8c300707719f8f88f8 to 89e8fd420f58ec3f5379e2a4c824803959a04590

Branch pushed to git repo; I updated commit sha1. New commits:

89e8fd4fix the date

comment:39 Changed 12 months ago by edgarcosta

  • Status changed from needs_review to positive_review

All the changes look good to me.

comment:40 Changed 11 months ago by vbraun

  • Branch changed from public/ticket/25366 to 89e8fd420f58ec3f5379e2a4c824803959a04590
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:41 Changed 11 months ago by embray

  • Milestone changed from sage-8.8 to sage-8.9

Not in Sage 8.8. Let's please to try keep tickets' milestones related to the release in which we actually intend to include them, and in particular the release in which they were actually included, especially when closing tickets.

Note: See TracTickets for help on using tickets.