Opened 4 years ago

Closed 4 years ago

#19437 closed defect (fixed)

SR.symbol: correct parent in inherting classes of SymbolicRing

Reported by: dkrenn Owned by:
Priority: major Milestone: sage-6.10
Component: symbolics Keywords:
Cc: Merged in:
Authors: Daniel Krenn Reviewers: Jeroen Demeyer
Report Upstream: N/A Work issues:
Branch: 9ac89ae (Commits) Commit: 9ac89ae33b6ee825c1a4179709c759406764d315
Dependencies: Stopgaps:

Description

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

Change History (10)

comment:1 Changed 4 years ago by dkrenn

  • Branch set to u/dkrenn/symbolic/sub-var

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

  • Authors set to Daniel Krenn
  • Commit set to e1aca778523496285eef6b12a7ae264b3edf3b7e
  • Status changed from new to needs_review

make ptestlong is currently running...


New commits:

e1aca77SR.symbol: set parent correctly (inheritance)

comment:3 in reply to: ↑ 2 Changed 4 years ago by dkrenn

Replying to dkrenn:

make ptestlong is currently running...

Passed, thus, now really needs review :)

comment:4 follow-up: Changed 4 years ago by jdemeyer

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.

comment:5 Changed 4 years ago by jdemeyer

  • Branch changed from u/dkrenn/symbolic/sub-var to u/jdemeyer/symbolic/sub-var

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

  • Commit changed from e1aca778523496285eef6b12a7ae264b3edf3b7e to 9ac89ae33b6ee825c1a4179709c759406764d315

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

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

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

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

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.

comment:9 Changed 4 years ago by jdemeyer

  • Reviewers set to Jeroen Demeyer
  • Status changed from needs_review to positive_review

comment:10 Changed 4 years ago by vbraun

  • Branch changed from u/jdemeyer/symbolic/sub-var to 9ac89ae33b6ee825c1a4179709c759406764d315
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.