Integer-valued polynomial ring #34988

fchapoton · 2023-02-06T17:04:50Z

Thanks for contributing to Sage! Detailed instructions to be added shortly.

Please make sure to also have a look at our
Code Style Conventions.

tscrim

The same comments apply for the PositiveBasis. It seems like there would be a benefit to avoid some duplication through an ABC.

tscrim · 2023-02-10T06:20:43Z

src/sage/rings/polynomial/integer_valued_polynomials.py

+            A = self.parent()
+            return A.sum(c * A.monomial(i + 1) for i, c in self)
+
+        def polynomial(self):


Should we cache this?

src/sage/rings/polynomial/integer_valued_polynomials.py

mkoeppe · 2023-02-10T06:41:41Z

#34814 (comment) is unresolved

fchapoton · 2023-02-19T20:31:34Z

Now in better state, working with realizations and only importing one thing into the global namespace

tscrim · 2023-02-20T02:34:19Z

@fchapoton Would you like me to go through what's there and leave some comments or are you still going to do some work on it?

fchapoton · 2023-02-20T07:55:37Z

Yes, please have a look if you can. I will not do anything else here for the next 7 days. Maybe moving some documentation to the top could be a good idea.

src/sage/rings/polynomial/integer_valued_polynomials.py

tscrim · 2023-02-23T03:50:02Z

src/sage/rings/polynomial/integer_valued_polynomials.py

+        S = self.S()
+        B = self.B()
+        B.module_morphism(S._from_binomial_basis, codomain=S).register_as_coercion()
+        S.module_morphism(B._from_shifted_basis, codomain=B).register_as_coercion()


This can lead to problems if B and S are garbage collected IIRC. This might be okay though as the realizations are stored and tied to the IVPR.

src/sage/rings/polynomial/integer_valued_polynomials.py

fchapoton · 2023-02-28T07:36:11Z

I wonder if I should try to factorise the "polynomial" and "from polynomial" methods ? I could create a _basis_polynomial method just returning the value of B[i] and S[i] as polynomials.

…eger_va

tscrim · 2023-03-02T01:05:22Z

I wonder if I should try to factorise the "polynomial" and "from polynomial" methods ? I could create a _basis_polynomial method just returning the value of B[i] and S[i] as polynomials.

I think that would be good. It took me a bit to actually see what the difference between those methods actually ways.

fchapoton · 2023-03-02T11:01:35Z

I wonder if I should try to factorise the "polynomial" and "from polynomial" methods ? I could create a _basis_polynomial method just returning the value of B[i] and S[i] as polynomials.

I think that would be good. It took me a bit to actually see what the difference between those methods actually ways.

Ok, I have now done both the factorisation of polynomial and from_polynomial and the better coercion. This should start to look good. Is there anything that I missed ?

tscrim · 2023-03-02T14:35:11Z

I think you have taken care of everything. The only comment I have on the recent commits is that I think you can simply return the module_morphism in _coerce_map_from_ so it is not recreated every time and that you know the base rings are compatible. Having it in the element constructor means it might pick up something going from, e.g., Z/5Z to Z/2Z as base rings.

fchapoton · 2023-03-02T15:29:25Z

I think you have taken care of everything. The only comment I have on the recent commits is that I think you can simply return the module_morphism in _coerce_map_from_ so it is not recreated every time and that you know the base rings are compatible. Having it in the element constructor means it might pick up something going from, e.g., Z/5Z to Z/2Z as base rings.

oh, I see, thanks. I did not know that one could return a morphism and not a boolean in _coerce_map_from_. Done and it seems to work fine enough.

tscrim · 2023-03-03T22:52:37Z

oh, I see, thanks. I did not know that one could return a morphism and not a boolean in _coerce_map_from_. Done and it seems to work fine enough.

It can also take an arbitrary Python callable object as well as I recall, which then gets wrapped into a coercion morphism.

Anyways, that's all my comments for now. Let me know when you are ready for a full review (which will probably be quite quick at this point).

fchapoton · 2023-03-06T08:12:40Z

ok, this should be almost ready.

tscrim

Just a few more little things.

src/sage/rings/polynomial/integer_valued_polynomials.py

github-actions · 2023-03-09T10:20:30Z

Documentation preview for this PR is ready! 🎉
Built with commit: 8774a72

fchapoton · 2023-03-09T15:10:55Z

Thanks, Travis. I think I have handled all your points in the latest commit.

tscrim

Thank you. LGTM.

fchapoton added the c: commutative algebra label Feb 6, 2023

kwankyu added p: major / 3 t: enhancement s: needs review labels Feb 7, 2023

fchapoton changed the title ~~U/chapoton/34814~~ Integer-valued polynomial ring Feb 7, 2023

This comment was marked as off-topic.

Sign in to view

tscrim requested changes Feb 10, 2023

View reviewed changes

fchapoton added s: needs work and removed s: needs review labels Feb 12, 2023

fchapoton force-pushed the u/chapoton/34814 branch from 207a954 to 79fbe52 Compare February 15, 2023 17:03

fchapoton marked this pull request as draft February 15, 2023 20:53

fchapoton added s: needs review and removed s: needs work labels Feb 19, 2023

tscrim reviewed Feb 23, 2023

View reviewed changes

fchapoton added 2 commits February 26, 2023 09:15

adding the ring of integer-valued polynomials

2a137f5

various details

46a9c24

fchapoton force-pushed the u/chapoton/34814 branch from bb38198 to 46a9c24 Compare February 26, 2023 09:54

fchapoton added 2 commits February 26, 2023 21:14

more fixes

b044dd3

little tweaks in doc, factoring some code

a29e33a

fchapoton added 5 commits February 28, 2023 08:48

tweak in doc

d95286b

remove a_realization, adding content, other small tweaks to doc

1bc29df

Merge branch 'u/chapoton/34814' of github.com:fchapoton/sage into int…

a60d8cc

…eger_va

sanity check for shift

001fdfd

moving some documentation to the top

a09d769

fchapoton added 3 commits March 2, 2023 08:22

refactoring polynomial and from_polynomial

95b8190

refactor coercion maps

a7f5913

minor tweaks in the doc

a8ae402

simplify coercion again

e28bd1a

fchapoton marked this pull request as ready for review March 6, 2023 08:13

tscrim requested changes Mar 9, 2023

View reviewed changes

fchapoton added 2 commits March 9, 2023 08:40

Merge branch 'develop' into u/chapoton/34814

223820d

reviewer's fixes

8774a72

tscrim approved these changes Mar 9, 2023

View reviewed changes

tscrim added s: positive review and removed s: needs review labels Mar 9, 2023

vbraun merged commit 1463bac into sagemath:develop Mar 13, 2023

mkoeppe added this to the sage-10.0 milestone Mar 13, 2023

mkoeppe removed the s: positive review label Mar 13, 2023

fchapoton deleted the u/chapoton/34814 branch July 16, 2023 19:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Integer-valued polynomial ring #34988

Integer-valued polynomial ring #34988

fchapoton commented Feb 6, 2023 •

edited

Loading

This comment was marked as off-topic.

tscrim left a comment

tscrim Feb 10, 2023

mkoeppe commented Feb 10, 2023

fchapoton commented Feb 19, 2023

tscrim commented Feb 20, 2023

fchapoton commented Feb 20, 2023

tscrim Feb 23, 2023

fchapoton commented Feb 28, 2023

tscrim commented Mar 2, 2023

fchapoton commented Mar 2, 2023

tscrim commented Mar 2, 2023

fchapoton commented Mar 2, 2023

tscrim commented Mar 3, 2023

fchapoton commented Mar 6, 2023

tscrim left a comment

github-actions bot commented Mar 9, 2023

fchapoton commented Mar 9, 2023

tscrim left a comment

Integer-valued polynomial ring #34988

Integer-valued polynomial ring #34988

Conversation

fchapoton commented Feb 6, 2023 • edited Loading

This comment was marked as off-topic.

tscrim left a comment

Choose a reason for hiding this comment

tscrim Feb 10, 2023

Choose a reason for hiding this comment

mkoeppe commented Feb 10, 2023

fchapoton commented Feb 19, 2023

tscrim commented Feb 20, 2023

fchapoton commented Feb 20, 2023

tscrim Feb 23, 2023

Choose a reason for hiding this comment

fchapoton commented Feb 28, 2023

tscrim commented Mar 2, 2023

fchapoton commented Mar 2, 2023

tscrim commented Mar 2, 2023

fchapoton commented Mar 2, 2023

tscrim commented Mar 3, 2023

fchapoton commented Mar 6, 2023

tscrim left a comment

Choose a reason for hiding this comment

github-actions bot commented Mar 9, 2023

fchapoton commented Mar 9, 2023

tscrim left a comment

Choose a reason for hiding this comment

fchapoton commented Feb 6, 2023 •

edited

Loading