Opened 5 years ago
Closed 4 years ago
#21353 closed enhancement (fixed)
fix `MIPVariable` inheritance
Reported by:  vdelecroix  Owned by:  

Priority:  major  Milestone:  sage8.2 
Component:  categories  Keywords:  
Cc:  tscrim, nthiery, SimonKing, mkoeppe  Merged in:  
Authors:  Vincent Delecroix, Travis Scrimshaw  Reviewers:  Vincent Delecroix, Travis Scrimshaw 
Report Upstream:  N/A  Work issues:  
Branch:  76c7fab (Commits, GitHub, GitLab)  Commit:  76c7fab2af36b139552bb2e92f0f710d8d608a26 
Dependencies:  #24096  Stopgaps: 
Description (last modified by )
Let MIPVariable
not inherit from Element
but SageObject
. The only reason it used to be this way is to be able to multiply variables with scalars that can be dealt with in other ways. The previous inheritance scheme used the old parent framework that we try to get rid of.
See also the task ticket: #21380
Change History (53)
comment:1 Changed 5 years ago by
 Branch set to u/vdelecroix/21353
 Commit set to aba5b7402c2f881f19e52c78cffb05974d33a2fe
 Status changed from new to needs_review
comment:2 Changed 5 years ago by
 Description modified (diff)
comment:3 Changed 5 years ago by
 Cc tscrim nthiery SimonKing added
comment:4 followup: ↓ 5 Changed 5 years ago by
+1 on getting rid of this kludge if we finally can!
About the change Parent.__init__
> Ring.__init__
: if this does not induce complications, I would tend to use the occasion to not have InfinityRing
inherit from Ring
and instead pass category=Rings()
to Parent.__init__
.
I don't expect a need for pure speed for this parent, right? Of course the elements could still belong to RingElement
.
comment:5 in reply to: ↑ 4 Changed 5 years ago by
Replying to nthiery:
About the change
Parent.__init__
>Ring.__init__
: if this does not induce complications, I would tend to use the occasion to not haveInfinityRing
inherit fromRing
and instead passcategory=Rings()
toParent.__init__
.
Doing that change would remove some functionality from InfinityRing
(although I highly, highly doubt anyone is creating things like ideals of the infinity ring). Although that would be something
I don't expect a need for pure speed for this parent, right? Of course the elements could still belong to
RingElement
.
I believe we have this parent (essentially) nailed in memory at startup because of oo
. So I don't think this is an issue at the end of the day.
comment:6 followup: ↓ 7 Changed 5 years ago by
It does introduce complications: removing the inheritance from Ring
causes error due to the absence of methods is_integral_domain
and is_field
. Some doctests are creating vector/matrices over the infinity ring.
comment:7 in reply to: ↑ 6 ; followup: ↓ 8 Changed 5 years ago by
Replying to vdelecroix:
It does introduce complications: removing the inheritance from
Ring
causes error due to the absence of methodsis_integral_domain
andis_field
. Some doctests are creating vector/matrices over the infinity ring.
Ah, shoot. That would be easy to fix by moving up those methods to
Rings
, which anyway we want to do at some point. But then that's
starting to shift beyond the scope of this ticket.
As an intermediate step, one could keep for now the inheritance from
Ring
, and pass a category=Rings()
argument to Parent
.
As you feel like guys!
comment:8 in reply to: ↑ 7 Changed 5 years ago by
 Dependencies set to #20686
I am setting #20686 as a dependency since it is very likely to create merge failures.
Replying to nthiery:
Replying to vdelecroix:
It does introduce complications: removing the inheritance from
Ring
causes error due to the absence of methodsis_integral_domain
andis_field
. Some doctests are creating vector/matrices over the infinity ring.Ah, shoot. That would be easy to fix by moving up those methods to
Rings
, which anyway we want to do at some point. But then that's starting to shift beyond the scope of this ticket.
If you do open a ticket for that, I would be happy to review.
As an intermediate step, one could keep for now the inheritance from
Ring
, and pass acategory=Rings()
argument toParent
.
Well be better to get rid of sage.structure.rings.Ring
in one step.
comment:9 Changed 5 years ago by
 Commit changed from aba5b7402c2f881f19e52c78cffb05974d33a2fe to 81506ffa3ebc3362b9b8035c50b5fbc5ff7f7e3e
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
81506ff  21353: remove guess_category

comment:10 followup: ↓ 11 Changed 5 years ago by
rebased
comment:11 in reply to: ↑ 10 Changed 5 years ago by
 Status changed from needs_review to needs_work
comment:12 Changed 5 years ago by
 Description modified (diff)
comment:13 Changed 5 years ago by
 Commit changed from 81506ffa3ebc3362b9b8035c50b5fbc5ff7f7e3e to 0e095c5a9923709bcd084a97ac6d0164080edd46
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
0e095c5  21353: remove guess_category

comment:15 Changed 5 years ago by
 Dependencies #20686 deleted
comment:16 Changed 5 years ago by
If this passes doctests, I have no objections. Nicolas?
comment:17 Changed 5 years ago by
 Status changed from needs_review to needs_work
Maybe two details: we should add a check like
if not isinstance(category, Category) raise TypeError(f"invalid category {category} for {self}")
(or at least check that category is not None
)
And remove the "or None" from the documentation line
 ``category``  a category, or list or tuple thereof, or ``None``
comment:18 followup: ↓ 21 Changed 5 years ago by
This modification makes Sage crash on startup...
TypeError: invalid category None for Parent of MIPVariables
comment:19 Changed 5 years ago by
 Status changed from needs_work to needs_review
comment:20 Changed 5 years ago by
Perhaps we should raise a warning? Also, I agree that we should remove None
from the doc because we should not initialize parents without categories, or have the default be Sets()
.
comment:21 in reply to: ↑ 18 Changed 5 years ago by
 Status changed from needs_review to needs_work
Replying to vdelecroix:
This modification makes Sage crash on startup...
TypeError: invalid category None for Parent of MIPVariables
That shows to me that this ticket isn't ready. I think we should only remove guess_category
when everything is actually initialized with a category.
comment:22 followups: ↓ 23 ↓ 25 Changed 5 years ago by
 Cc mkoeppe added
Matthias, do you know if MIPVariables
needs to be a parent, and if so, could the category for that parent be simply changed to Sets
?
comment:23 in reply to: ↑ 22 Changed 5 years ago by
Replying to tscrim:
Matthias, do you know if
MIPVariables
needs to be a parent
A Sage Element
class requires a Parent
class so I guess the answer is "yes" here.
could the category for that parent be simply changed to
Sets
?
Probably also "yes".
comment:24 Changed 5 years ago by
So, if I explicit pass Sets
as the category for that, make set_generic
have a default category of Sets
, I still have a can of worms between changing the doctests (trivial) to the more nontrivial parent of interfaces, e.g., I get:
TypeError: invalid category None for C library interface to GAP
What are people's thoughts on making the default category for Parent
be Sets
. At least this seemed to generate less individual issues...
comment:25 in reply to: ↑ 22 Changed 5 years ago by
Replying to tscrim:
Matthias, do you know if
MIPVariables
needs to be a parent, and if so, could the category for that parent be simply changed toSets
?
A MIPVariable
is a strange thing at the moment. In particular it is mutable. Currently there is a parent class called MIPVariableParent
. Not sure where it fits with regards to categories.
comment:26 followup: ↓ 27 Changed 5 years ago by
As far as I understand, the only reason why MIPVariables
is a parent is because it benefits from coercion (like scalar multiplication). And as far as I know, the only way to use coercion is to use the Category
/Parent
/Element
framework.
comment:27 in reply to: ↑ 26 Changed 5 years ago by
Replying to vdelecroix:
As far as I understand, the only reason why
MIPVariables
is a parent is because it benefits from coercion (like scalar multiplication). And as far as I know, the only way to use coercion is to use theCategory
/Parent
/Element
framework.
I think we could just lift the necessary parts directly into MIPVariables
as it is an action, and IIRC, the coercion framework has some support for things that are not parents (e.g., python int
). That would make a good followup ticket.
comment:28 followup: ↓ 29 Changed 4 years ago by
I have no idea why, but it seems that the problems on this ticket no longer occur. I am deprecating guess_category
on #24109, so this ticket can be closed as duplicate.
comment:29 in reply to: ↑ 28 Changed 4 years ago by
 Dependencies set to #24109
Replying to jdemeyer:
so this ticket can be closed as duplicate.
...unless you want to keep some of the other changes that you did on this ticket.
comment:30 followup: ↓ 31 Changed 4 years ago by
The only current changes I'd really want to keep are in rings/infinity.py
so infinity rings properly declare their categories.
Also, from looking at the code for MIPVariables
, the parent probably should be removed and instead MIPVariable
just implemented as __mul__
and __rmul__
as a subclass of SageObject
.
Want me to do both of those things here on this ticket and make this independent of #24109?
comment:31 in reply to: ↑ 30 ; followup: ↓ 32 Changed 4 years ago by
comment:32 in reply to: ↑ 31 Changed 4 years ago by
Replying to jdemeyer:
Replying to tscrim:
Want me to do both of those things here on this ticket and make this independent of #24109?
I don't know who you are asking...
More to you since you did #24109.
I would say: feel free to do that or to close this ticket.
I will go ahead and do that tomorrow (I am now on Australia time) at some point.
comment:33 Changed 4 years ago by
 Branch changed from u/vdelecroix/21353 to public/categoires/some_cleanup21353
 Commit changed from 0e095c5a9923709bcd084a97ac6d0164080edd46 to c87eaae21e436a6f21a2443937ad9d38aa2e6d7f
 Milestone changed from sage7.4 to sage8.1
 Status changed from needs_work to needs_review
New branch that removes the parent for MIP variables and does the Ring.__init__
for initializing the category of infinity rings.
New commits:
3cf11a9  Always enable bad_parent_warnings

7c9dcd6  Made the infinity rings call Ring.__init__ to initialize the category.

c87eaae  Removing useless parent/element framework for MIP variables.

comment:34 Changed 4 years ago by
return NotImplemented
instead of raise TypeError
. Also I would avoid the name self
in arithmetic slot functions: use left
and right
instead of self
and m
.
comment:35 Changed 4 years ago by
 Commit changed from c87eaae21e436a6f21a2443937ad9d38aa2e6d7f to 403ba9ed800d0af80d0d871ab4ef556cfe5484db
Branch pushed to git repo; I updated commit sha1. New commits:
403ba9e  Return NotImplemented and use arg names left/right.

comment:36 Changed 4 years ago by
Both good points. Changed.
comment:37 Changed 4 years ago by
Don't use from sage.matrix.matrix import is_Matrix
. Import it from sage.structure.element
instead. See #24096
comment:38 Changed 4 years ago by
 Status changed from needs_review to needs_work
comment:39 Changed 4 years ago by
 Commit changed from 403ba9ed800d0af80d0d871ab4ef556cfe5484db to f1fc3650ab4a5935dde9bff373ffe61d49caba44
comment:40 Changed 4 years ago by
 Dependencies changed from #24109 to #24109 #24096
 Status changed from needs_work to needs_review
Right...somehow I should put 2 and 2 together and get 4, Sage tells me that is true. :P
comment:41 Changed 4 years ago by
comment:42 followup: ↓ 43 Changed 4 years ago by
I didn't see how that was possible because this moves two calls to is_Matrix
and their imports (even if it was not consolidated into 1 toplevel import). Really #24096 is positively reviewed, so it's not a big deal.
comment:43 in reply to: ↑ 42 Changed 4 years ago by
Replying to tscrim:
I didn't see how that was possible because this moves two calls to
is_Matrix
and their imports
OK, I thought that you only added imports in this patch, but you did indeed move an import.
comment:44 Changed 4 years ago by
 Dependencies changed from #24109 #24096 to #24096
 Status changed from needs_review to needs_work
Merge conflict. Note that you may want to wait until the next beta when #24096 is merged.
comment:45 Changed 4 years ago by
 Description modified (diff)
 Summary changed from get rid of guess_category to Various fixes in category initialization
comment:46 Changed 4 years ago by
 Commit changed from f1fc3650ab4a5935dde9bff373ffe61d49caba44 to 5897f6da7233574728df6a9501a241ca59e28d35
Branch pushed to git repo; I updated commit sha1. New commits:
5897f6d  Merge branch 'public/categoires/some_cleanup21353' of git://trac.sagemath.org/sage into public/categoires/some_cleanup21353

comment:47 Changed 4 years ago by
 Milestone changed from sage8.1 to sage8.2
 Status changed from needs_work to needs_review
Rebased
comment:48 Changed 4 years ago by
 Description modified (diff)
comment:49 Changed 4 years ago by
 Summary changed from Various fixes in category initialization to fix `MIPVariable` inheritance
comment:50 Changed 4 years ago by
 Commit changed from 5897f6da7233574728df6a9501a241ca59e28d35 to 76c7fab2af36b139552bb2e92f0f710d8d608a26
Branch pushed to git repo; I updated commit sha1. New commits:
76c7fab  21353: extra doctest

comment:51 Changed 4 years ago by
 Reviewers set to Vincent Delecroix
Added an extra doctest for checking about the base ring being taken into account.
I am happy with the current version.
comment:52 Changed 4 years ago by
 Reviewers changed from Vincent Delecroix to Vincent Delecroix, Travis Scrimshaw
 Status changed from needs_review to positive_review
LGTM, thanks.
comment:53 Changed 4 years ago by
 Branch changed from public/categoires/some_cleanup21353 to 76c7fab2af36b139552bb2e92f0f710d8d608a26
 Resolution set to fixed
 Status changed from positive_review to closed
New commits:
21353: remove guess_category