fix MIPVariable inheritance

[videlec]

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

Depends on #24096

CC: @tscrim @nthiery @simon-king-jena @mkoeppe

Component: categories

Author: Vincent Delecroix, Travis Scrimshaw

Branch/Commit: 76c7fab

Reviewer: Vincent Delecroix, Travis Scrimshaw

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

[videlec]

Commit: aba5b74

[videlec]

New commits:

aba5b74

21353: remove guess_category

[videlec]

Branch: u/vdelecroix/21353

[nthiery]

comment:4

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

[tscrim]

comment:5

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.

[videlec]

comment:6

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.

[nthiery]

comment:7

Replying to @videlec:

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!

[videlec]

comment:8

I am setting #20686 as a dependency since it is very likely to create merge failures.

Replying to @nthiery:

Replying to @videlec:

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.

[videlec]

Dependencies: #20686

[sagetrac-git]

Changed commit from aba5b74 to 81506ff

[sagetrac-git]

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

81506ff

21353: remove guess_category

[videlec]

comment:10

rebased

[jdemeyer]

comment:11

Replying to @videlec:

rebased

...on an old version of Sage.

Still needs rebase.

[sagetrac-git]

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

0e095c5

21353: remove guess_category

[sagetrac-git]

Changed commit from 81506ff to 0e095c5

[videlec]

comment:14

rebased on rc1

[jdemeyer]

Changed dependencies from #20686 to none

[jdemeyer]

comment:16

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

[jdemeyer]

comment:17

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``

[tscrim]

comment:42

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.

[jdemeyer]

comment:43

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.

[jdemeyer]

Changed dependencies from #24109 #24096 to #24096

[jdemeyer]

comment:44

Merge conflict. Note that you may want to wait until the next beta when #24096 is merged.

[sagetrac-git]

Changed commit from f1fc365 to 5897f6d

[sagetrac-git]

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

5897f6d

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

[tscrim]

comment:47

Rebased

[sagetrac-git]

Changed commit from 5897f6d to 76c7fab

[sagetrac-git]

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

76c7fab

21353: extra doctest

[videlec]

Reviewer: Vincent Delecroix

[videlec]

comment:51

Added an extra doctest for checking about the base ring being taken into account.

I am happy with the current version.

[tscrim]

comment:52

LGTM, thanks.

[tscrim]

Changed reviewer from Vincent Delecroix to Vincent Delecroix, Travis Scrimshaw

[vbraun]

Changed branch from public/categoires/some_cleanup-21353 to 76c7fab