Opened 6 years ago
Closed 5 years ago
#19259 closed enhancement (fixed)
subrings of the symbolic ring
Reported by:  dkrenn  Owned by:  

Priority:  major  Milestone:  sage6.9 
Component:  symbolics  Keywords:  
Cc:  behackl, cheuberg  Merged in:  
Authors:  Daniel Krenn  Reviewers:  Benjamin Hackl 
Report Upstream:  N/A  Work issues:  
Branch:  d376b10 (Commits, GitHub, GitLab)  Commit:  d376b1022bc5f7aa5b8ffbb422cfe0bfa0d951d5 
Dependencies:  Stopgaps: 
Description
We create subrings of the symbolic ring, which allow or reject a given set of variables.
Change History (27)
comment:1 Changed 6 years ago by
 Branch set to u/dkrenn/symbolicsubring
comment:2 followup: ↓ 3 Changed 6 years ago by
 Commit set to 7938d95efbba4282216166d9ba200e1552a9b21f
comment:3 in reply to: ↑ 2 ; followup: ↓ 4 Changed 6 years ago by
 Status changed from new to needs_review
Replying to rws:
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?)
comment:4 in reply to: ↑ 3 Changed 6 years ago by
Replying to dkrenn:
Replying to rws:
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.
comment:5 Changed 6 years ago by
 Cc behackl cheuberg added
comment:6 followup: ↓ 7 Changed 6 years ago by
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 nonobvious, than perhaps there should be a shortcut for it SR.subring_of_constants()
?
comment:7 in reply to: ↑ 6 Changed 6 years ago by
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 noncoercion part ;)
I am only not quite sure that
only_constants
deserves to be a parameter: it can be implemented by accepting no variables and ifSR.subring(accepting_variables=())
looks bad and nonobvious, than perhaps there should be a shortcut for itSR.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).
comment:8 followup: ↓ 9 Changed 6 years ago by
Ah, with constant you mean no symbols (maybe better clarify). Yeah I make that distinction in #19040 often enough so it seems very natural.
comment:9 in reply to: ↑ 8 ; followup: ↓ 10 Changed 6 years ago by
comment:10 in reply to: ↑ 9 ; followup: ↓ 12 Changed 6 years ago by
Replying to dkrenn:
Replying to rws:
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.
comment:11 Changed 6 years ago by
 Commit changed from 7938d95efbba4282216166d9ba200e1552a9b21f to 852959a8bec1f6da8c5a2f0d69edb8b23b9d0c59
Branch pushed to git repo; I updated commit sha1. New commits:
852959a  rename only_constants > no_variables

comment:12 in reply to: ↑ 10 Changed 6 years ago by
comment:13 followup: ↓ 14 Changed 6 years ago by
 Commit changed from 852959a8bec1f6da8c5a2f0d69edb8b23b9d0c59 to e4837e9b4f2e9edff62ae0bb192a406c44cba561
Branch pushed to git repo; I updated commit sha1. New commits:
e4837e9  correct parent of result of an_element

comment:14 in reply to: ↑ 13 Changed 6 years ago by
comment:15 followup: ↓ 17 Changed 5 years ago by
Hello! I started reviewing this ticket, and for now, I have the following comments:
is_variable_valid
>has_valid_variable
orallows_variable
? This would be a semantic improvement as the current version does not really fit to the otherparent.is_*
methods, IMHO. While it is OK that the subring factory checks that every element in the
vars
tuple can be thrown intoSR
inSymbolicSubringFactory.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 inGenericSymbolicSubring.__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.
comment:16 Changed 5 years ago by
 Commit changed from e4837e9b4f2e9edff62ae0bb192a406c44cba561 to 581f3157f57c8b38390cf515e065cf62eacec306
comment:17 in reply to: ↑ 15 Changed 5 years ago by
Replying to behackl:
Hello! I started reviewing this ticket, and for now, I have the following comments:
is_variable_valid
>has_valid_variable
orallows_variable
? This would be a semantic improvement as the current version does not really fit to the otherparent.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 intoSR
inSymbolicSubringFactory.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 inGenericSymbolicSubring.__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.
comment:18 followups: ↓ 19 ↓ 20 Changed 5 years ago by
 Branch changed from u/dkrenn/symbolicsubring to u/behackl/symbolics/symbolicsubring
 Commit changed from 581f3157f57c8b38390cf515e065cf62eacec306 to c4a0e226b7fa34959d990be3bb768d270fd7f9a5
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/symbolicsubring

c4a0e22  merge accepting and rejecting functors in all cases

comment:19 in reply to: ↑ 18 ; followup: ↓ 21 Changed 5 years ago by
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 followup 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.
comment:20 in reply to: ↑ 18 ; followup: ↓ 23 Changed 5 years ago by
Replying to behackl:
Besides some minor changes to the code and documentation,
Crossreview 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
comment:21 in reply to: ↑ 19 Changed 5 years ago by
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 followup 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 followup ticket.
comment:22 Changed 5 years ago by
 Commit changed from c4a0e226b7fa34959d990be3bb768d270fd7f9a5 to d376b1022bc5f7aa5b8ffbb422cfe0bfa0d951d5
Branch pushed to git repo; I updated commit sha1. New commits:
d376b10  revert changes to merge of functors

comment:23 in reply to: ↑ 20 Changed 5 years ago by
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 pleasegit 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 followup ticket.
As you have reviewed my changes, I'll set this to positive_review
after building and checking the documentation again.
comment:24 followup: ↓ 25 Changed 5 years ago by
 Status changed from needs_review to positive_review
comment:25 in reply to: ↑ 24 Changed 5 years ago by
Reviewer(s), please insert your name(s) in the corresponding field of the ticket.
comment:26 Changed 5 years ago by
 Reviewers set to Benjamin Hackl
comment:27 Changed 5 years ago by
 Branch changed from u/behackl/symbolics/symbolicsubring to d376b1022bc5f7aa5b8ffbb422cfe0bfa0d951d5
 Resolution set to fixed
 Status changed from positive_review to closed
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:
write workaround for #19231
extend _coerce_map_from_ in rejectingsubring
write doctests for coercions, common_parent, pushout
minor docstring rewriting of factory
subring in index.rst
simplify a doctest
change ValueError to TypeError to make everything work with SR as it should
typo in docstring
docstring of SR.subring
module description of subring