#25366 closed enhancement (fixed)
Expose the function intervalproducts from Harvey's hypellfrob
Reported by:  alexjbest  Owned by:  

Priority:  major  Milestone:  sage8.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
 Branch set to u/alexjbest/intervalproducts
comment:2 Changed 2 years ago by
 Commit set to 96e07833778c9d050b53c79b56ba903ff350d514
 Status changed from new to needs_review
comment:3 Changed 2 years ago by
 Branch changed from u/alexjbest/intervalproducts to u/alexjbest/intervalproductsnowrap
 Commit changed from 96e07833778c9d050b53c79b56ba903ff350d514 to 6bbe6c01c6bcf56fe05aa7119444285de47a6d64
 Dependencies #25365 deleted
 Status changed from needs_review to needs_work
comment:4 followups: ↓ 5 ↓ 11 Changed 2 years ago by
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 ; followup: ↓ 6 Changed 2 years ago by
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 ; followup: ↓ 8 Changed 2 years ago by
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
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).
comment:8 in reply to: ↑ 6 ; followup: ↓ 12 Changed 2 years ago by
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 followup: ↓ 14 Changed 2 years ago by
Avoid the iteration syntax 0 <= k < numintervals
, Cython understands range()
.
The existing NTL code also has a tendency to overuse 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
 Commit changed from 6bbe6c01c6bcf56fe05aa7119444285de47a6d64 to be6948f676a22dd4ab62287ff57306799066b4da
Branch pushed to git repo; I updated commit sha1. New commits:
be6948f  operator > get

comment:11 in reply to: ↑ 4 ; followup: ↓ 13 Changed 2 years ago by
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:
be6948f  operator > get

comment:12 in reply to: ↑ 8 Changed 2 years ago by
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 ; followup: ↓ 15 Changed 2 years ago by
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 ; followup: ↓ 19 Changed 2 years ago by
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
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++ styleIt 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
 Commit changed from be6948f676a22dd4ab62287ff57306799066b4da to c5f70461f6bc2dc78c95f0c808da0cbbd5539299
Branch pushed to git repo; I updated commit sha1. New commits:
c5f7046  cleanup based on comments of jdemeyer

comment:17 Changed 2 years ago by
 Commit changed from c5f70461f6bc2dc78c95f0c808da0cbbd5539299 to 154ffb1c26269bdef9c44993c1ba8780eb917f33
Branch pushed to git repo; I updated commit sha1. New commits:
154ffb1  correctly support matrices which don't fit into linbox float

comment:18 Changed 22 months ago by
 Milestone changed from sage8.3 to sage8.4
update milestone 8.3 > 8.4
comment:19 in reply to: ↑ 14 Changed 21 months ago by
 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 lineA.SetDims(m._nrows, m._ncols)
toset_ntl_matrix_modn_dense
and call that with an existing matrix likecdef 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
@jdemeyer any update on this? still seems to merge fine, patchbot still passing!
comment:21 Changed 19 months ago by
 Commit changed from 154ffb1c26269bdef9c44993c1ba8780eb917f33 to e0a3749936382b21a0adc456b8933242e69048bc
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
a277dc0  testing some

39bc447  🎶 I can call my code with no wrapping class, no wrapping class, no wrapping class 🎵

9dd0727  operator > get

147cf8c  cleanup based on comments of jdemeyer

0eefa0f  correctly support matrices which don't fit into linbox float

e0a3749  fix docpdf

comment:22 Changed 19 months ago by
 Milestone changed from sage8.4 to sage8.5
comment:23 Changed 18 months ago by
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
 Cc jdemeyer added
comment:25 Changed 16 months ago by
 Milestone changed from sage8.5 to sage8.7
comment:26 Changed 16 months ago by
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
There is a potential overflow in hypellfrob_interval_products_wrapper
: you use int
where NTL uses long
.
comment:28 Changed 16 months ago by
Check you divisions: add from __future__ import division
and use //
where you want floor division.
comment:29 Changed 16 months ago by
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
 Milestone changed from sage8.7 to sage8.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
 Commit changed from e0a3749936382b21a0adc456b8933242e69048bc to dc91a183fbace87e1ad7e684e07e48b351b51adc
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
e28420f  testing some

3ae91aa  🎶 I can call my code with no wrapping class, no wrapping class, no wrapping class 🎵

ad065be  operator > get

d55279f  cleanup based on comments of jdemeyer

5e4d7dc  correctly support matrices which don't fit into linbox float

965d5de  fix docpdf

dc91a18  pep8ify

comment:32 Changed 12 months ago by
 Branch changed from u/alexjbest/intervalproductsnowrap to public/ticket/25366
 Commit changed from dc91a183fbace87e1ad7e684e07e48b351b51adc to 54e89297ecfcb1c621f070fb83acefe0c2dc56df
I have made a fresh, rebased and squashed branch.
New commits:
54e8929  Expose the function intervalproducts from Harvey's hypellfrob

comment:33 Changed 12 months ago by
 Status changed from needs_review to needs_work
This seems to be currently broken.
comment:34 Changed 12 months ago by
Broken, apparently in 8.7.rc0, maybe due to #27070 ?
comment:35 Changed 12 months ago by
 Commit changed from 54e89297ecfcb1c621f070fb83acefe0c2dc56df to 0d3f8f6ea5d15d47d18b56cb30487ceaa795c15a
comment:36 Changed 12 months ago by
 Commit changed from 0d3f8f6ea5d15d47d18b56cb30487ceaa795c15a to c0bb7c208b0974ab3582ca8c300707719f8f88f8
Branch pushed to git repo; I updated commit sha1. New commits:
c0bb7c2  trac 25366 some details in code and doc

comment:37 Changed 12 months ago by
 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
 Commit changed from c0bb7c208b0974ab3582ca8c300707719f8f88f8 to 89e8fd420f58ec3f5379e2a4c824803959a04590
Branch pushed to git repo; I updated commit sha1. New commits:
89e8fd4  fix the date

comment:39 Changed 12 months ago by
 Status changed from needs_review to positive_review
All the changes look good to me.
comment:40 Changed 11 months ago by
 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
 Milestone changed from sage8.8 to sage8.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.
New commits:
wrap ntl_mat_ZZ_p functionality
wrap harvey's intervalproducts