subrings of the symbolic ring #19259

dkrenn · 2015-09-20T17:40:33Z

We create subrings of the symbolic ring, which allow or reject a given set of variables.

CC: @behackl @cheuberg

Component: symbolics

Author: Daniel Krenn

Branch/Commit: d376b10

Reviewer: Benjamin Hackl

Issue created by migration from https://trac.sagemath.org/ticket/19259

The text was updated successfully, but these errors were encountered:

dkrenn · 2015-09-20T17:40:55Z

Branch: u/dkrenn/symbolic-subring

rwst · 2015-09-21T08:47:40Z

comment:2

The (unregistered, yours?) patchbot seems to have problems, but probably not with this branch. I'm in favor of this enhancement as it allows some solutions to other tickets.

Last 10 new commits:

`4b8f9be`	`write workaround for #19231`
`8696a6e`	extend `_coerce_map_from_` in rejecting-subring
`30f7058`	`write doctests for coercions, common_parent, pushout`
`a54b4af`	`minor docstring rewriting of factory`
`fb2885c`	`subring in index.rst`
`eab9513`	`simplify a doctest`
`23352b2`	`change ValueError to TypeError to make everything work with SR as it should`
`2d8b7c4`	`typo in docstring`
`15ec40e`	`docstring of SR.subring`
`7938d95`	`module description of subring`

rwst · 2015-09-21T08:47:40Z

Commit: 7938d95

dkrenn · 2015-09-21T11:54:07Z

comment:3

Replying to @rwst:

The (unregistered, yours?) patchbot seems to have problems, but probably not with this branch.

Yes, it is my patchbot. I think something related to the upgrade to beta7 ...

I'm in favor of this enhancement as it allows some solutions to other tickets.

Good :)

(Which tickets are these?)

rwst · 2015-09-21T12:39:04Z

comment:4

Replying to @dkrenn:

Replying to @rwst:

I'm in favor of this enhancement as it allows some solutions to other tickets.

Good :)

(Which tickets are these?)

See comment 57 of #12967 by vbraun: "The symbolic constants, like pi.pyobject(), should be elements in some parent set. The symbolic constants can then coerce into the infinity ring, solving the pi.pyobject() < oo == False issue."

Consequently this would be good for #19040.

novoselt · 2015-09-21T17:18:22Z

comment:6

This is fantastic! Don't plan to review it as I am not an expert on coersion stuff, but looks very good. I am only not quite sure that only_constants deserves to be a parameter: it can be implemented by accepting no variables and if SR.subring(accepting_variables=()) looks bad and non-obvious, than perhaps there should be a shortcut for it SR.subring_of_constants()?

dkrenn · 2015-09-22T07:17:24Z

comment:7

Replying to @novoselt:

This is fantastic!

Good to hear :)

Don't plan to review it as I am not an expert on coersion stuff, but looks very good.

You could review the non-coercion part ;)

I am only not quite sure that only_constants deserves to be a parameter: it can be implemented by accepting no variables and if SR.subring(accepting_variables=()) looks bad and non-obvious, than perhaps there should be a shortcut for it SR.subring_of_constants()?

My feeling is that some kind of shortcut to this special ring is convenient to have (and it is also good to mention this subring explicitly somewhere). Thus, +1 that there is some kind of explicit way to create the subring of symbolic constants.
However, I am open on how this should be achieved. My indention was having one method subring which can create all kind of subrings (cf. Unix philosophy: Do One Thing and Do It Well).

rwst · 2015-09-22T07:48:27Z

comment:8

Ah, with constant you mean no symbols (maybe better clarify). Yeah I make that distinction in #19040 often enough so it seems very natural.

dkrenn · 2015-09-22T08:04:23Z

comment:9

Replying to @rwst:

Ah, with constant you mean no symbols (maybe better clarify). Yeah I make that distinction in #19040 often enough so it seems very natural.

Keyword only_constants --> no_symbols ? (And maybe all occurrences of variable --> symbol? What is the best terminology?)

rwst · 2015-09-22T08:25:47Z

comment:10

Replying to @dkrenn:

Replying to @rwst:

Ah, with constant you mean no symbols (maybe better clarify). Yeah I make that distinction in #19040 often enough so it seems very natural.

Keyword only_constants --> no_symbols ?

Or no_variables.

(And maybe all occurrences of variable --> symbol?

Not necessary IMO.

sagetrac-git · 2015-09-22T09:04:39Z

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

`852959a`	`rename only_constants --> no_variables`

sagetrac-git · 2015-09-22T09:04:39Z

Changed commit from 7938d95 to 852959a

dkrenn · 2015-09-22T09:05:51Z

comment:12

Replying to @rwst:

Keyword only_constants --> no_symbols ?

Or no_variables.

Fits good; renamed.

sagetrac-git · 2015-10-19T13:05:20Z

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

`e4837e9`	`correct parent of result of an_element`

sagetrac-git · 2015-10-19T13:05:20Z

Changed commit from 852959a to e4837e9

dkrenn · 2015-10-19T13:17:51Z

comment:14

Replying to @sagetrac-git:

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

`e4837e9`	`correct parent of result of an_element`

Small commit to correct the parent. Needs_review.

behackl · 2016-01-22T01:59:57Z

comment:15

Hello! I started reviewing this ticket, and for now, I have the following comments:

is_variable_valid --> has_valid_variable or allows_variable? This would be a semantic improvement as the current version does not really fit to the other parent.is_*-methods, IMHO.
While it is OK that the subring factory checks that every element in the vars-tuple can be thrown into SR in SymbolicSubringFactory.create_key_and_extra_args, the elements are not checked for being valid identifiers. I'd suggest to do so either in the factory, or directly in GenericSymbolicSubring.__init__. Otherwise, this is possible:

sage: SR.subring(accepting_variables=(0, pi, sqrt(2), I))
Symbolic Subring accepting the variables 0, I, pi, sqrt(2)

I also have some minor changes (language etc.) which I will push as soon as I am done with my review.

sagetrac-git · 2016-01-23T09:05:14Z

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

`ff99c7f`	`Trac #19259: change to has_valid_variable`
`581f315`	`Trac #19259: check validity of variables`

sagetrac-git · 2016-01-23T09:05:14Z

Changed commit from e4837e9 to 581f315

dkrenn · 2016-01-23T09:06:59Z

comment:17

Replying to @behackl:

Hello! I started reviewing this ticket, and for now, I have the following comments:

is_variable_valid --> has_valid_variable or allows_variable? This would be a semantic improvement as the current version does not really fit to the other parent.is_*-methods, IMHO.

I agree; changed.

While it is OK that the subring factory checks that every element in the vars-tuple can be thrown into SR in SymbolicSubringFactory.create_key_and_extra_args, the elements are not checked for being valid identifiers. I'd suggest to do so either in the factory, or directly in GenericSymbolicSubring.__init__.

Added check.

Otherwise, this is possible:

sage: SR.subring(accepting_variables=(0, pi, sqrt(2), I))
Symbolic Subring accepting the variables 0, I, pi, sqrt(2)

Doctest added.

behackl · 2016-01-23T11:42:55Z

Changed commit from 581f315 to c4a0e22

behackl · 2016-01-23T11:42:55Z

comment:18

Thanks, this resolves these two issues.

Besides some minor changes to the code and documentation, I noticed that the merge methods for the functors were doing the wrong thing:

merging two accepting functors should give an accepting functor with the intersection of the variables,
merging two rejecting functors should give a rejecting functor with the union of the variables,
when merging a rejecting and an accepting functor, the result has to to be an accepting functor with all accepted variables, but without the rejected ones.

I adapted the code accordingly.

In principle, this would be a positive_review from my side, however, I'd like to rewrite your code some more such that subrings of subrings can be constructed as well. The easiest way to do so (probably), is to let the functors do everything.

Last 10 new commits:

`7938d95`	`module description of subring`
`852959a`	`rename only_constants --> no_variables`
`e4837e9`	`correct parent of result of an_element`
`ff99c7f`	`Trac #19259: change to has_valid_variable`
`581f315`	`Trac #19259: check validity of variables`
`c9b2428`	`improve language`
`05dc834`	`misc. changes, indentation, line breaks`
`8cc884a`	`fix merge of functors`
`aeae8f3`	`Merge tag '7.0' into symbolics/symbolic-subring`
`c4a0e22`	`merge accepting and rejecting functors in all cases`

behackl · 2016-01-23T11:42:55Z

Changed branch from u/dkrenn/symbolic-subring to u/behackl/symbolics/symbolic-subring

cheuberg · 2016-01-23T13:54:27Z

comment:19

Replying to @behackl:

In principle, this would be a positive_review from my side, however, I'd like to rewrite your code some more such that subrings of subrings can be constructed as well. The easiest way to do so (probably), is to let the functors do everything.

The question is whether to postpone the rewriting to a follow-up ticket (i.e., does your planned rewriting affect the interface or is it just internal rewriting which will then allow new things to be done?) This also in view of several tickets depending on this ticket here.

dkrenn · 2016-01-23T16:11:03Z

comment:20

Replying to @behackl:

Besides some minor changes to the code and documentation,

Cross-review of the minor things positive, but ...

I noticed that the merge methods for the functors were doing the wrong thing:

merging two accepting functors should give an accepting functor with the intersection of the variables,

merging two rejecting functors should give a rejecting functor with the union of the variables,

when merging a rejecting and an accepting functor, the result has to to be an accepting functor with all accepted variables, but without the rejected ones.

No, merge is correct. Composition means creating a larger subring, which contains both the corresonding rings (it is used in pushout).
So please git revert


8cc884a	`fix merge of functors`

and


c4a0e22	`merge accepting and rejecting functors in all cases`

dkrenn · 2016-01-23T16:11:45Z

comment:21

Replying to @cheuberg:

Replying to @behackl:

In principle, this would be a positive_review from my side, however, I'd like to rewrite your code some more such that subrings of subrings can be constructed as well. The easiest way to do so (probably), is to let the functors do everything.

The question is whether to postpone the rewriting to a follow-up ticket (i.e., does your planned rewriting affect the interface or is it just internal rewriting which will then allow new things to be done?) This also in view of several tickets depending on this ticket here.

+1 for postpone to follow-up ticket.

sagetrac-git · 2016-01-23T16:29:49Z

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

`d376b10`	`revert changes to merge of functors`

sagetrac-git · 2016-01-23T16:29:49Z

Changed commit from c4a0e22 to d376b10

behackl · 2016-01-23T16:37:05Z

comment:23

Replying to @dkrenn:

No, merge is correct. Composition means creating a larger subring, which contains both the corresonding rings (it is used in pushout).
So please git revert

Done. I thought of composition instead of something that is used in the construction of the pushout, sorry.

My approach requires an implemented compoisiton of functors; I'll take care of that in a follow-up ticket.

As you have reviewed my changes, I'll set this to positive_review after building and checking the documentation again.

dkrenn · 2016-01-23T17:54:03Z

comment:25

Reviewer(s), please insert your name(s) in the corresponding field of the ticket.

cheuberg · 2016-01-23T18:24:39Z

Reviewer: Benjamin Hackl

vbraun · 2016-01-24T12:52:50Z

Changed branch from u/behackl/symbolics/symbolic-subring to d376b10

dkrenn added this to the sage-6.9 milestone Sep 20, 2015

dkrenn added c: symbolics labels Sep 20, 2015

dkrenn added the s: needs review label Sep 21, 2015

behackl added s: positive review and removed s: needs review labels Jan 23, 2016

vbraun removed the s: positive review label Jan 24, 2016

vbraun closed this as completed in 27a7d27 Jan 24, 2016

behackl mentioned this issue Dec 29, 2022

Meta ticket: Asymptotic Expansions in SageMath #17601

Open

This was referenced Jan 24, 2016

common generators for asymptotic expansions #19306

Closed

wrong inverse action when using ConstructionFunctor.coercion_reversed #19521

Closed

behackl mentioned this issue Jan 7, 2023

composition of symbolic subring functors #19949

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

subrings of the symbolic ring #19259

subrings of the symbolic ring #19259

dkrenn commented Sep 20, 2015

dkrenn commented Sep 20, 2015

rwst commented Sep 21, 2015

rwst commented Sep 21, 2015

dkrenn commented Sep 21, 2015

rwst commented Sep 21, 2015

novoselt commented Sep 21, 2015

dkrenn commented Sep 22, 2015

rwst commented Sep 22, 2015

dkrenn commented Sep 22, 2015

rwst commented Sep 22, 2015

sagetrac-git mannequin commented Sep 22, 2015

sagetrac-git mannequin commented Sep 22, 2015

dkrenn commented Sep 22, 2015

sagetrac-git mannequin commented Oct 19, 2015

sagetrac-git mannequin commented Oct 19, 2015

dkrenn commented Oct 19, 2015

behackl commented Jan 22, 2016

sagetrac-git mannequin commented Jan 23, 2016

sagetrac-git mannequin commented Jan 23, 2016

dkrenn commented Jan 23, 2016

behackl commented Jan 23, 2016

behackl commented Jan 23, 2016

behackl commented Jan 23, 2016

cheuberg commented Jan 23, 2016

dkrenn commented Jan 23, 2016

dkrenn commented Jan 23, 2016

sagetrac-git mannequin commented Jan 23, 2016

sagetrac-git mannequin commented Jan 23, 2016

behackl commented Jan 23, 2016

dkrenn commented Jan 23, 2016

cheuberg commented Jan 23, 2016

vbraun commented Jan 24, 2016

subrings of the symbolic ring #19259

subrings of the symbolic ring #19259

Comments

dkrenn commented Sep 20, 2015

dkrenn commented Sep 20, 2015

rwst commented Sep 21, 2015

rwst commented Sep 21, 2015

dkrenn commented Sep 21, 2015

rwst commented Sep 21, 2015

novoselt commented Sep 21, 2015

dkrenn commented Sep 22, 2015

rwst commented Sep 22, 2015

dkrenn commented Sep 22, 2015

rwst commented Sep 22, 2015

sagetrac-git mannequin commented Sep 22, 2015

sagetrac-git mannequin commented Sep 22, 2015

dkrenn commented Sep 22, 2015

sagetrac-git mannequin commented Oct 19, 2015

sagetrac-git mannequin commented Oct 19, 2015

dkrenn commented Oct 19, 2015

behackl commented Jan 22, 2016

sagetrac-git mannequin commented Jan 23, 2016

sagetrac-git mannequin commented Jan 23, 2016

dkrenn commented Jan 23, 2016

behackl commented Jan 23, 2016

behackl commented Jan 23, 2016

behackl commented Jan 23, 2016

cheuberg commented Jan 23, 2016

dkrenn commented Jan 23, 2016

dkrenn commented Jan 23, 2016

sagetrac-git mannequin commented Jan 23, 2016

sagetrac-git mannequin commented Jan 23, 2016

behackl commented Jan 23, 2016

dkrenn commented Jan 23, 2016

cheuberg commented Jan 23, 2016

vbraun commented Jan 24, 2016