Opened 3 years ago

Closed 2 years ago

#27315 closed enhancement (fixed)

Minor optimizations to libsingular polynomials

Reported by: mmezzarobba Owned by:
Priority: minor Milestone: sage-9.1
Component: basic arithmetic Keywords:
Cc: Merged in:
Authors: Marc Mezzarobba Reviewers: Frédéric Chapoton
Report Upstream: N/A Work issues:
Branch: 41fe6dd (Commits, GitHub, GitLab) Commit: 41fe6dd7965ee6867fc8c62dd1a52ee98f5de383
Dependencies: Stopgaps:

Status badges

Description


Change History (20)

comment:1 Changed 3 years ago by mmezzarobba

  • Branch set to u/mmezzarobba/mpoly_minor_opt
  • Commit set to b1f807e5cbef7a401fcc5b929e2355e1414374a4
  • Dependencies set to #26067
  • Status changed from new to needs_review

New commits:

​b86fea5#26067 Speed up exact division in ℤ[x,y,...] (again)
​5082de3minor cleanup
​701387bmpoly //: minor optimization
​b1f807e#27315 minor optimizations to libsingular mpolys

comment:2 Changed 3 years ago by git

  • Commit changed from b1f807e5cbef7a401fcc5b929e2355e1414374a4 to 47a457ce6a626b60a51dda89f8af1e5b9706a5f3

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

​47a457c#27315 minor optimizations to libsingular mpolys

comment:3 Changed 3 years 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:4 Changed 3 years ago by embray

  • Milestone changed from sage-8.8 to sage-8.9

Moving tickets from the Sage 8.8 milestone that have been actively worked on in the last six months to the next release milestone (optimistically).

comment:5 Changed 2 years ago by git

  • Commit changed from 47a457ce6a626b60a51dda89f8af1e5b9706a5f3 to 35e9cad932c1c39e84244ed4114a9c527921c098

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

​35e9cad#27315 minor optimizations to libsingular mpolys

comment:6 Changed 2 years ago by mmezzarobba

  • Dependencies #26067 deleted

rebased

comment:7 follow-up: Changed 2 years ago by chapoton

not convinced by the use of "isinstance" here: 

-        if self._parent._base.is_finite() and self._parent._base.characteristic() > 1<<29:
+        if n_GetChar(r) > 1<<29 and isinstance(self._parent._base, FiniteField):

comment:8 in reply to: ↑ 7 Changed 2 years ago by mmezzarobba

  • Status changed from needs_review to needs_work

Replying to chapoton:

not convinced by the use of "isinstance" here:

Ouch—I don't remember at all why I made this change, but I agree that it looks strange. Thank you.

comment:9 Changed 2 years ago by git

  • Commit changed from 35e9cad932c1c39e84244ed4114a9c527921c098 to 85e31dc66134b0bdb2637024fb492357ab820035

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

​85e31dc#27315 minor optimizations to libsingular mpolys

comment:10 Changed 2 years ago by mmezzarobba

  • Status changed from needs_work to needs_review

Ok, I removed the redundant test, let's see if the patchbot complains.

comment:11 Changed 2 years ago by embray

  • Milestone changed from sage-8.9 to sage-9.1

Ticket retargeted after milestone closed

comment:12 Changed 2 years ago by chapoton

  • Status changed from needs_review to needs_work

red branch => needs work

comment:13 Changed 2 years ago by git

  • Commit changed from 85e31dc66134b0bdb2637024fb492357ab820035 to f8cd16194a7ee7d4adf482cb07b2b6c79d07f812

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

​f8cd161#27315 minor optimizations to libsingular mpolys

comment:14 Changed 2 years ago by mmezzarobba

  • Status changed from needs_work to needs_review

comment:15 Changed 2 years ago by chapoton

The added import 

+from sage.rings.finite_rings.finite_field_base cimport FiniteField

seems not to be used.

comment:16 follow-up: Changed 2 years ago by chapoton

and about the change of "is_monomial", is this really the same ? I mean, does "monomial" mean coefficient 1 ?

comment:17 Changed 2 years ago by git

  • Commit changed from f8cd16194a7ee7d4adf482cb07b2b6c79d07f812 to 41fe6dd7965ee6867fc8c62dd1a52ee98f5de383

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

​41fe6dd#27315 minor optimizations to libsingular mpolys

comment:18 in reply to: ↑ 16 Changed 2 years ago by mmezzarobba

Replying to chapoton:

The added import 

+from sage.rings.finite_rings.finite_field_base cimport FiniteField

seems not to be used.

Fixed, thank you.

Replying to chapoton:

and about the change of "is_monomial", is this really the same ? I mean, does "monomial" mean coefficient 1 ?

Yes.

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

comment:19 Changed 2 years ago by chapoton

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

ok, then let it be. Merci

comment:20 Changed 2 years ago by vbraun

  • Branch changed from u/mmezzarobba/mpoly_minor_opt to 41fe6dd7965ee6867fc8c62dd1a52ee98f5de383
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.