Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SR.symbol: correct parent in inherting classes of SymbolicRing #19437

Closed
dkrenn opened this issue Oct 20, 2015 · 13 comments
Closed

SR.symbol: correct parent in inherting classes of SymbolicRing #19437

dkrenn opened this issue Oct 20, 2015 · 13 comments

Comments

@dkrenn
Copy link
Contributor

dkrenn commented Oct 20, 2015

sage: from sage.symbolic.ring import SymbolicRing
sage: class MySymbolicRing(SymbolicRing):
....:     def _repr_(self):
....:         return 'My Symbolic Ring'
sage: MySR = MySymbolicRing()
sage: MySR.var('x').parent()
Symbolic Ring

Component: symbolics

Author: Daniel Krenn

Branch/Commit: 9ac89ae

Reviewer: Jeroen Demeyer

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

@dkrenn dkrenn added this to the sage-6.10 milestone Oct 20, 2015
@dkrenn
Copy link
Contributor Author

dkrenn commented Oct 20, 2015

Branch: u/dkrenn/symbolic/sub-var

@dkrenn
Copy link
Contributor Author

dkrenn commented Oct 20, 2015

comment:2

make ptestlong is currently running...


New commits:

e1aca77SR.symbol: set parent correctly (inheritance)

@dkrenn
Copy link
Contributor Author

dkrenn commented Oct 20, 2015

Commit: e1aca77

@dkrenn
Copy link
Contributor Author

dkrenn commented Oct 20, 2015

Author: Daniel Krenn

@dkrenn
Copy link
Contributor Author

dkrenn commented Oct 20, 2015

comment:3

Replying to @dkrenn:

make ptestlong is currently running...

Passed, thus, now really needs review :)

@jdemeyer
Copy link

jdemeyer commented Nov 2, 2015

comment:4

Can you tell me what's your use case for this branch?

I am a bit worried about

  1. the impact on current code, in particular CallableSymbolicExpressionRing_class.
  2. conflicts in case different parents use variables with the same name, see the global pynac_symbol_registry.

@jdemeyer
Copy link

jdemeyer commented Nov 2, 2015

Changed branch from u/dkrenn/symbolic/sub-var to u/jdemeyer/symbolic/sub-var

@jdemeyer
Copy link

jdemeyer commented Nov 2, 2015

comment:6

Have a look at my extra commit which makes symbol names specific to a parent. I'm not really sure that this is what you want since I don't know your use-case.


New commits:

9ac89aeMove pynac_symbol_registry to cdef attribute SR.symbols

@jdemeyer
Copy link

jdemeyer commented Nov 2, 2015

Changed commit from e1aca77 to 9ac89ae

@dkrenn
Copy link
Contributor Author

dkrenn commented Nov 3, 2015

comment:7

Replying to @jdemeyer:

Can you tell me what's your use case for this branch?

Basically, I came long this with #19259, which implements subrings of the symbolic ring. A symbolic subring is inheriting from the symbolic ring class. The element_constructor is overridden in the following way: It calls the element_constructor of SR and then checks if the element's variables are "valid".

I had a short look at your changes and they seem to do what I need, but I'll have a more careful check later (I'm kind of busy right now).

Thanks

Daniel

@dkrenn
Copy link
Contributor Author

dkrenn commented Nov 11, 2015

comment:8

Replying to @jdemeyer:

Have a look at my extra commit which makes symbol names specific to a parent. I'm not really sure that this is what you want since I don't know your use-case.

Yout changes look good to me and do what this ticket claims. So from my side a positive review.

@jdemeyer
Copy link

Reviewer: Jeroen Demeyer

@vbraun
Copy link
Member

vbraun commented Nov 12, 2015

Changed branch from u/jdemeyer/symbolic/sub-var to 9ac89ae

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants