Opened 4 years ago

Closed 3 years ago

#19259 closed enhancement (fixed)

subrings of the symbolic ring

Reported by: dkrenn Owned by:
Priority: major Milestone: sage-6.9
Component: symbolics Keywords:
Cc: behackl, cheuberg Merged in:
Authors: Daniel Krenn Reviewers: Benjamin Hackl
Report Upstream: N/A Work issues:
Branch: d376b10 (Commits) 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 4 years ago by dkrenn

  • Branch set to u/dkrenn/symbolic-subring

comment:2 follow-up: Changed 4 years ago by rws

  • Commit set to 7938d95efbba4282216166d9ba200e1552a9b21f

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:

4b8f9bewrite workaround for #19231
8696a6eextend _coerce_map_from_ in rejecting-subring
30f7058write doctests for coercions, common_parent, pushout
a54b4afminor docstring rewriting of factory
fb2885csubring in index.rst
eab9513simplify a doctest
23352b2change ValueError to TypeError to make everything work with SR as it should
2d8b7c4typo in docstring
15ec40edocstring of SR.subring
7938d95module description of subring

comment:3 in reply to: ↑ 2 ; follow-up: Changed 4 years ago by dkrenn

  • 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 4 years ago by rws

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 4 years ago by dkrenn

  • Cc behackl cheuberg added

comment:6 follow-up: Changed 4 years ago by novoselt

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()?

comment:7 in reply to: ↑ 6 Changed 4 years ago by dkrenn

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).

comment:8 follow-up: Changed 4 years ago by 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.

comment:9 in reply to: ↑ 8 ; follow-up: Changed 4 years ago by 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 ? (And maybe all occurrences of variable --> symbol? What is the best terminology?)

comment:10 in reply to: ↑ 9 ; follow-up: Changed 4 years ago by rws

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 4 years ago by git

  • Commit changed from 7938d95efbba4282216166d9ba200e1552a9b21f to 852959a8bec1f6da8c5a2f0d69edb8b23b9d0c59

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

852959arename only_constants --> no_variables

comment:12 in reply to: ↑ 10 Changed 4 years ago by dkrenn

Replying to rws:

Keyword only_constants --> no_symbols ?

Or no_variables.

Fits good; renamed.

comment:13 follow-up: Changed 4 years ago by git

  • Commit changed from 852959a8bec1f6da8c5a2f0d69edb8b23b9d0c59 to e4837e9b4f2e9edff62ae0bb192a406c44cba561

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

e4837e9correct parent of result of an_element

comment:14 in reply to: ↑ 13 Changed 4 years ago by dkrenn

Replying to git:

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

e4837e9correct parent of result of an_element

Small commit to correct the parent. Needs_review.

comment:15 follow-up: Changed 3 years ago by 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.
  • 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.

comment:16 Changed 3 years ago by git

  • Commit changed from e4837e9b4f2e9edff62ae0bb192a406c44cba561 to 581f3157f57c8b38390cf515e065cf62eacec306

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

ff99c7fTrac #19259: change to has_valid_variable
581f315Trac #19259: check validity of variables

comment:17 in reply to: ↑ 15 Changed 3 years ago by dkrenn

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.

comment:18 follow-ups: Changed 3 years ago by behackl

  • Branch changed from u/dkrenn/symbolic-subring to u/behackl/symbolics/symbolic-subring
  • 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:

7938d95module description of subring
852959arename only_constants --> no_variables
e4837e9correct parent of result of an_element
ff99c7fTrac #19259: change to has_valid_variable
581f315Trac #19259: check validity of variables
c9b2428improve language
05dc834misc. changes, indentation, line breaks
8cc884afix merge of functors
aeae8f3Merge tag '7.0' into symbolics/symbolic-subring
c4a0e22merge accepting and rejecting functors in all cases

comment:19 in reply to: ↑ 18 ; follow-up: Changed 3 years ago by 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.

comment:20 in reply to: ↑ 18 ; follow-up: Changed 3 years ago by dkrenn

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

8cc884afix merge of functors

and

c4a0e22merge accepting and rejecting functors in all cases

comment:21 in reply to: ↑ 19 Changed 3 years ago by dkrenn

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.

comment:22 Changed 3 years ago by git

  • Commit changed from c4a0e226b7fa34959d990be3bb768d270fd7f9a5 to d376b1022bc5f7aa5b8ffbb422cfe0bfa0d951d5

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

d376b10revert changes to merge of functors

comment:23 in reply to: ↑ 20 Changed 3 years ago by behackl

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.

comment:24 follow-up: Changed 3 years ago by behackl

  • Status changed from needs_review to positive_review

comment:25 in reply to: ↑ 24 Changed 3 years ago by dkrenn

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

comment:26 Changed 3 years ago by cheuberg

  • Reviewers set to Benjamin Hackl

comment:27 Changed 3 years ago by vbraun

  • Branch changed from u/behackl/symbolics/symbolic-subring to d376b1022bc5f7aa5b8ffbb422cfe0bfa0d951d5
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.