Opened 5 years ago

Closed 4 years ago

#21353 closed enhancement (fixed)

fix `MIPVariable` inheritance

Reported by: vdelecroix Owned by:
Priority: major Milestone: sage-8.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:

Status badges

Description (last modified by vdelecroix)

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 vdelecroix

  • Branch set to u/vdelecroix/21353
  • Commit set to aba5b7402c2f881f19e52c78cffb05974d33a2fe
  • Status changed from new to needs_review

New commits:

aba5b7421353: remove guess_category

comment:2 Changed 5 years ago by vdelecroix

  • Description modified (diff)

comment:3 Changed 5 years ago by tscrim

  • Cc tscrim nthiery SimonKing added

comment:4 follow-up: Changed 5 years ago by nthiery

+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 tscrim

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 have InfinityRing inherit from Ring and instead pass category=Rings() to Parent.__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 follow-up: Changed 5 years ago by vdelecroix

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 ; follow-up: Changed 5 years ago by nthiery

Replying to vdelecroix:

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.

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 vdelecroix

  • 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 methods is_integral_domain and is_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 a category=Rings() argument to Parent.

Well be better to get rid of sage.structure.rings.Ring in one step.

comment:9 Changed 5 years ago by git

  • Commit changed from aba5b7402c2f881f19e52c78cffb05974d33a2fe to 81506ffa3ebc3362b9b8035c50b5fbc5ff7f7e3e

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

81506ff21353: remove guess_category

comment:10 follow-up: Changed 5 years ago by vdelecroix

rebased

comment:11 in reply to: ↑ 10 Changed 5 years ago by jdemeyer

  • Status changed from needs_review to needs_work

Replying to vdelecroix:

rebased

...on an old version of Sage.

Still needs rebase.

comment:12 Changed 5 years ago by jdemeyer

  • Description modified (diff)

comment:13 Changed 5 years ago by git

  • Commit changed from 81506ffa3ebc3362b9b8035c50b5fbc5ff7f7e3e to 0e095c5a9923709bcd084a97ac6d0164080edd46

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

0e095c521353: remove guess_category

comment:14 Changed 5 years ago by vdelecroix

  • Status changed from needs_work to needs_review

rebased on rc1

comment:15 Changed 5 years ago by jdemeyer

  • Dependencies #20686 deleted

comment:16 Changed 5 years ago by jdemeyer

If this passes doctests, I have no objections. Nicolas?

comment:17 Changed 5 years ago by jdemeyer

  • 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 follow-up: Changed 5 years ago by vdelecroix

This modification makes Sage crash on startup...

TypeError: invalid category None for Parent of MIPVariables
Last edited 5 years ago by vdelecroix (previous) (diff)

comment:19 Changed 5 years ago by vdelecroix

  • Status changed from needs_work to needs_review

comment:20 Changed 5 years ago by tscrim

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 jdemeyer

  • 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 follow-ups: Changed 5 years ago by tscrim

  • 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 jdemeyer

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 tscrim

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 non-trivial 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 mkoeppe

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 to Sets?

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 follow-up: Changed 5 years ago by 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 the Category/Parent/Element framework.

Last edited 5 years ago by vdelecroix (previous) (diff)

comment:27 in reply to: ↑ 26 Changed 5 years ago by tscrim

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 the Category/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 follow-up ticket.

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

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 jdemeyer

  • 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 follow-up: Changed 4 years ago by tscrim

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

I would say: feel free to do that or to close this ticket.

comment:32 in reply to: ↑ 31 Changed 4 years ago by tscrim

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 tscrim

  • Authors changed from Vincent Delecroix to Vincent Delecroix, Travis Scrimshaw
  • Branch changed from u/vdelecroix/21353 to public/categoires/some_cleanup-21353
  • Commit changed from 0e095c5a9923709bcd084a97ac6d0164080edd46 to c87eaae21e436a6f21a2443937ad9d38aa2e6d7f
  • Milestone changed from sage-7.4 to sage-8.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:

3cf11a9Always enable bad_parent_warnings
7c9dcd6Made the infinity rings call Ring.__init__ to initialize the category.
c87eaaeRemoving useless parent/element framework for MIP variables.

comment:34 Changed 4 years ago by jdemeyer

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 git

  • Commit changed from c87eaae21e436a6f21a2443937ad9d38aa2e6d7f to 403ba9ed800d0af80d0d871ab4ef556cfe5484db

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

403ba9eReturn NotImplemented and use arg names left/right.

comment:36 Changed 4 years ago by tscrim

Both good points. Changed.

comment:37 Changed 4 years ago by jdemeyer

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 jdemeyer

  • Status changed from needs_review to needs_work

comment:39 Changed 4 years ago by git

  • Commit changed from 403ba9ed800d0af80d0d871ab4ef556cfe5484db to f1fc3650ab4a5935dde9bff373ffe61d49caba44

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

8d59a70Deprecate sage.matrix.matrix
f1fc365Rebase over #24096.

comment:40 Changed 4 years ago by tscrim

  • 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 jdemeyer

I didn't say that this has to depend on #24096. I only asked to write it in such a way that it doesn't conflict with #24096.

comment:42 follow-up: Changed 4 years ago by tscrim

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 top-level import). Really #24096 is positively reviewed, so it's not a big deal.

comment:43 in reply to: ↑ 42 Changed 4 years ago by jdemeyer

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 jdemeyer

  • 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 jdemeyer

  • Description modified (diff)
  • Summary changed from get rid of guess_category to Various fixes in category initialization

comment:46 Changed 4 years ago by git

  • Commit changed from f1fc3650ab4a5935dde9bff373ffe61d49caba44 to 5897f6da7233574728df6a9501a241ca59e28d35

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

5897f6dMerge branch 'public/categoires/some_cleanup-21353' of git://trac.sagemath.org/sage into public/categoires/some_cleanup-21353

comment:47 Changed 4 years ago by tscrim

  • Milestone changed from sage-8.1 to sage-8.2
  • Status changed from needs_work to needs_review

Rebased

comment:48 Changed 4 years ago by vdelecroix

  • Description modified (diff)

comment:49 Changed 4 years ago by vdelecroix

  • Summary changed from Various fixes in category initialization to fix `MIPVariable` inheritance

comment:50 Changed 4 years ago by git

  • Commit changed from 5897f6da7233574728df6a9501a241ca59e28d35 to 76c7fab2af36b139552bb2e92f0f710d8d608a26

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

76c7fab21353: extra doctest

comment:51 Changed 4 years ago by vdelecroix

  • 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 tscrim

  • 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 vbraun

  • Branch changed from public/categoires/some_cleanup-21353 to 76c7fab2af36b139552bb2e92f0f710d8d608a26
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.