sagemath / sage

Main repository of SageMath
https://www.sagemath.org
Other
1.43k stars 479 forks source link

Ring not considered module over itself #31713

Open mjungmath opened 3 years ago

mjungmath commented 3 years ago

We have the following bug:

sage: P = QQ[x]
sage: P in Modules(QQ)
True
sage: P in Modules(P)
False
sage: Modules(P)(P)
---------------------------------------------------------------------------
NotImplementedError                       Traceback (most recent call last)
...
NotImplementedError:

but on the other hand

sage: P^1 in Modules(P)
True

works.

Depends on #31721

CC: @tscrim @mkoeppe @dimpase @slel @DaveWitteMorris

Component: categories

Branch/Commit: u/gh-mjungmath/ring_not_considered_module_over_itself @ 0ef2060

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

slel commented 3 years ago
comment:1

Something to document rather than to fix, if you ask me.

Similarly, a field is not in the category of vector spaces even though it is a vector space over itself.

sage: QQ in VectorSpaces(QQ)
False
sage: QQ^1 in VectorSpaces(QQ)
True
mjungmath commented 3 years ago
comment:2

I would like to define a morphisms in the category of modules from a ring to a module over that ring. But now we get:

sage: Hom(QQ, QQ^2, category=Modules(QQ))
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
...
ValueError: Rational Field is not in Category of vector spaces over Rational Field

Any way to circumvent this then?

Similarly for the dual btw.

tscrim commented 3 years ago
comment:3

I don't consider this to be a bug, but a consequence of how we often secretly equate things in mathematics. QQ doesn't know it is a 1-dimensional module because it doesn't know how to do linear algebra (albeit it is trivial). So I also believe it should be documented and does not need a fix.

It seems like you just need to be a little more careful with what objects you are working with if you want it to be a module morphism (rather than an action).

mjungmath commented 3 years ago
comment:4

Replying to @tscrim:

I don't consider this to be a bug, but a consequence of how we often secretly equate things in mathematics. QQ doesn't know it is a 1-dimensional module because it doesn't know how to do linear algebra (albeit it is trivial). So I also believe it should be documented and does not need a fix.

It seems like you just need to be a little more careful with what objects you are working with if you want it to be a module morphism (rather than an action).

But that doesn't answer my question how I can define a morphism (i.e. module homomorphism) from a ring to a module over the same ring. This must be somehow possible in Sage.

Fine, this works with QQ when you convert it into a vector space via QQ^1 but this trick doesn't work for e.g. scalar fields anymore.

slel commented 3 years ago
comment:5

Can you turn the scalar field with scalars in some ring R into a vector field with vectors in R^1?

mjungmath commented 3 years ago
comment:6

Replying to @slel:

Can you turn the scalar field with scalars in some ring R into a vector field with vectors in R^1?

As far as I know, you can't, unfortunately. And I don't think there is a need to change anything or add this features. The only thing I need is a module homomorphism starting/ending from/to the scalar field algebra. It's really annoying that this is not working...

slel commented 3 years ago
comment:7

Despite my initial comment, I'm fine with making each ring a module over itself.

(Given how little I know about categories in Sage, I should listen more than speak.)

tscrim commented 3 years ago
comment:8

Replying to @mjungmath:

Replying to @tscrim:

I don't consider this to be a bug, but a consequence of how we often secretly equate things in mathematics. QQ doesn't know it is a 1-dimensional module because it doesn't know how to do linear algebra (albeit it is trivial). So I also believe it should be documented and does not need a fix.

It seems like you just need to be a little more careful with what objects you are working with if you want it to be a module morphism (rather than an action).

But that doesn't answer my question how I can define a morphism (i.e. module homomorphism) from a ring to a module over the same ring. This must be somehow possible in Sage.

If you want a more hack way around it, you can modify the __contains__ for the modules. However, I don't like equating things implicitly. QQ does not know it is a 1-dimensional module over itself; it doesn't have that structure. In particular, you do not have the guarantee of certain methods being implemented. This will cause massive problems when you start using it as a module.

Again, if you want a module morphism, then you need to construct it as a module. Otherwise you have to have a ring morphism (which is what is used to define the action of QQ on QQ-modules).

Fine, this works with QQ when you convert it into a vector space via QQ^1 but this trick doesn't work for e.g. scalar fields anymore.

That sounds more like a problem with the implementation of scalar fields because the free module code should work with any field as linear algebra works over any field. Note that QQ^1 is just syntactic sugar for FreeModule(QQ, 1).

mjungmath commented 3 years ago
comment:9

This makes total sense.

What about a coercion that exactly does this, coercing a ring R to FreeModule(R, 1) under the category coercion Modules(R)(R)?

mjungmath commented 3 years ago

Branch: u/gh-mjungmath/ring_not_considered_module_over_itself

mjungmath commented 3 years ago

Commit: a815b71

mjungmath commented 3 years ago
comment:11

I have uploaded a draft for the coercion. Please take a look.


New commits:

a815b71Trac #31713: coerce rings into modules
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 3 years ago

Changed commit from a815b71 to e68b8a4

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 3 years ago

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

e68b8a4Trac #31713: map=False to make vector space coercion work
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 3 years ago

Changed commit from e68b8a4 to 0ef2060

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 3 years ago

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

0ef2060Trac #31713: unify to map=False
mjungmath commented 3 years ago
comment:14

The last step, set map=False, was necessary to make Modules(QQ)(QQ) work in cases where free_module or vector_space do not provide the optional argument map. In addition, this unifies the global behavior of free_module and vector_space methods due to the principle: if you want more, you have to state more.

mjungmath commented 3 years ago
comment:15

I have another idea. I think, we need a __pow__ method for general parents that delegate to _pow_ in the categories. So far, the method _pow_int in categories.fields has no purpose at all.

Then we deal with the coercion via self**1.

mjungmath commented 3 years ago

Dependencies: #31721

tscrim commented 3 years ago
comment:17

To be very clear about this, this is not a coercion, but a conversion. I also don't think we should simply check that x in _Rings because what if x is not a subring of the base ring? I think we should explicitly check that x is self.base() or x in self.base() (the latter being when the base is a category).

Changing the default behavior of free_module is a regression that will break code in the wild and requires at least a deprecation message.

I don't like perma-nailing categories into memory. This should be avoided as much as possible. I highly doubt the calls to those categories in free_module.py are a source of slowness. If they already are nailed in memory, you should import from there. I am also think we should keep using the is_foo() functions there too unless you have a good reason to change this in this ticket.

mjungmath commented 3 years ago
comment:18

Replying to @tscrim:

I don't like perma-nailing categories into memory. This should be avoided as much as possible. I highly doubt the calls to those categories in free_module.py are a source of slowness. If they already are nailed in memory, you should import from there. I am also think we should keep using the is_foo() functions there too unless you have a good reason to change this in this ticket.

The reason is that for example

sage: M = Manifold(2, 'M')
sage: A = M.scalar_field_algebra()
sage: FreeModule(A, 1)

won't work otherwise. Rings that do not inherit from ring.pyx don't admit the necessary methods. Perhaps it is a better idea to move all those methods to the category...?

As for perma-nailing, that wasn't my idea. It happens everywhere in the code.

nbruin commented 3 years ago
comment:19

Replying to @mjungmath:

As for perma-nailing, that wasn't my idea. It happens everywhere in the code.

One should indeed try to avoid instantiating instanced of parametrized categories (such as categories that have a base ring and/or dimensions) because they can lead to unbounded memory use over time.

See #15801. There are good solutions for that: Rather than Modules(QQ) use Modules(Fields()). The base ring can be obtained from the object itself anyway, so it doesn't need to be stored on its category. Note that objects can have their category "refined", so Modules(QQ) can still be instantiated if absolutely wanted by the user, and then an object can be made to have that category. The system shouldn't instantiate those categories uninvited, though. This has caused real computational failures in CRT-based computations (there are a lot of finite fields ...), so this is not some theoretical problem.

It may well be that subsequent developers were not aware of the real issues addressed in #15801 and, inspired by a desire for mathematical precision, have reintroduced automatic instantiation of parametrized categories. These are bugs that should be corrected.

tscrim commented 3 years ago
comment:20

Replying to @mjungmath:

Replying to @tscrim:

I don't like perma-nailing categories into memory. This should be avoided as much as possible. I highly doubt the calls to those categories in free_module.py are a source of slowness. If they already are nailed in memory, you should import from there. I am also think we should keep using the is_foo() functions there too unless you have a good reason to change this in this ticket.

The reason is that for example

sage: M = Manifold(2, 'M')
sage: A = M.scalar_field_algebra()
sage: FreeModule(A, 1)

won't work otherwise. Rings that do not inherit from ring.pyx don't admit the necessary methods. Perhaps it is a better idea to move all those methods to the category...?

Then add these methods to make it work as it suggests an incomplete implementation.

As for perma-nailing, that wasn't my idea. It happens everywhere in the code.

Then import it from those locations because if we decide to stop nailing it in, then we only have to change it in one place and then fix the errors in all of the files. Do not nail anything more than what already is done. Just because it is done elsewhere does not make it a good practice.

mjungmath commented 3 years ago
comment:21

Replying to @tscrim:

Replying to @mjungmath:

Replying to @tscrim:

I don't like perma-nailing categories into memory. This should be avoided as much as possible. I highly doubt the calls to those categories in free_module.py are a source of slowness. If they already are nailed in memory, you should import from there. I am also think we should keep using the is_foo() functions there too unless you have a good reason to change this in this ticket.

The reason is that for example

sage: M = Manifold(2, 'M')
sage: A = M.scalar_field_algebra()
sage: FreeModule(A, 1)

won't work otherwise. Rings that do not inherit from ring.pyx don't admit the necessary methods. Perhaps it is a better idea to move all those methods to the category...?

Then add these methods to make it work as it suggests an incomplete implementation.

Either way: the category Rings must be refactored then. If a ring must have these methods, this must be stated in the category, e.g. via abstract methods or halfway implementations. See #31722.

As for perma-nailing, that wasn't my idea. It happens everywhere in the code.

Then import it from those locations because if we decide to stop nailing it in, then we only have to change it in one place and then fix the errors in all of the files. Do not nail anything more than what already is done. Just because it is done elsewhere does not make it a good practice.

That can be done in #31722.

tscrim commented 3 years ago
comment:22

That does not follow. It is not a category implementation but a class that is using those parents. There is no refactoring needed nor documentation needed to be added there.

Now there is a good reason to nail Fields(). There are a lot of people (by my understanding) that use code that works over prime numbers as the prime (or its power) vary in tight loops. For this, the difference between a specific category instance and checking the cache matters. There might be some other things that require a similar treatment, but just doing it arbitrarily is not good.

nbruin commented 3 years ago
comment:23

Replying to @tscrim:

I don't like perma-nailing categories into memory. This should be avoided as much as possible. I highly doubt the calls to those categories in free_module.py are a source of slowness. If they already are nailed in memory, you should import from there. I am also think we should keep using the is_foo() functions there too unless you have a good reason to change this in this ticket.

Do you have a more objective reason than just taste? I agree that linking categories willy-nilly is not a good idea, but for the most part I don't think it's going to cause memory leaks: if I recall correctly, most categories are immortal once they get created anyway. So the leak is creating them in the first place :-).

For categories like Fields(), though, this is certainly not an issue: they're fundamentally immortal. Perhaps the namespace clutter of binding them to _Fields somewhere isn't so nice, but I don't think it will have a large memory impact: it's just one binding (each time) in the name space dictionary of a module. A method likely has a much more severe memory impact, because it needs to get bound for each object that gets instantiated.

It may well be that using R.is_fields() leads to better/more readable/more efficient code than R in _Fields, or that it's better to spell the latter as R in Fields() (even though it's necessarily a bit slower), but I don't think "perma nailing" by itself is bad here: it's just a binding to an object that exists anyway, much like the binding from ... import ... would create, or for that matter import ..., which binds a name in a namespace to a module that (has just been caused to) exist.

tscrim commented 3 years ago
comment:24

Replying to @nbruin:

Replying to @tscrim:

I don't like perma-nailing categories into memory. This should be avoided as much as possible. I highly doubt the calls to those categories in free_module.py are a source of slowness. If they already are nailed in memory, you should import from there. I am also think we should keep using the is_foo() functions there too unless you have a good reason to change this in this ticket.

Do you have a more objective reason than just taste? I agree that linking categories willy-nilly is not a good idea, but for the most part I don't think it's going to cause memory leaks: if I recall correctly, most categories are immortal once they get created anyway. So the leak is creating them in the first place :-).

Indeed, categories should not be immortal. Although some of them might effectively be that way since we have standard rings and fields in Sage that nobody would suggest we do lazily (ZZ, QQ, etc.).

For categories like Fields(), though, this is certainly not an issue: they're fundamentally immortal. Perhaps the namespace clutter of binding them to _Fields somewhere isn't so nice, but I don't think it will have a large memory impact: it's just one binding (each time) in the name space dictionary of a module. A method likely has a much more severe memory impact, because it needs to get bound for each object that gets instantiated.

For the non-parameterized categories, this is less of an issue, but I don't want to allow more use of this without a good reason. Most importantly, I would want to do this in one place and one place only. Everything else should just be an import of that specific instance.

It may well be that using R.is_fields() leads to better/more readable/more efficient code than R in _Fields, or that it's better to spell the latter as R in Fields() (even though it's necessarily a bit slower), but I don't think "perma nailing" by itself is bad here: it's just a binding to an object that exists anyway, much like the binding from ... import ... would create, or for that matter import ..., which binds a name in a namespace to a module that (has just been caused to) exist.

I agree generally, but I don't want this to do be done in many places and make it seem like it is a good practice. It makes it harder to refactor and other developers might start to think they can do this for any category they want, which I don't think we should advocate. Which they then move onto more parents being allowed. I know this is a slippery-slope argument, so it doesn't have much merit, but it is something I am a bit concerned about.

nbruin commented 3 years ago
comment:25

Replying to @tscrim:

Indeed, categories should not be immortal. Although some of them might effectively be that way since we have standard rings and fields in Sage that nobody would suggest we do lazily (ZZ, QQ, etc.).

If I recall correctly, categories are fundamentally immortal once they participate in the dynamic classes that are derived from them (isn't this always?) The category hierarchy is just too involved to be reliably fully dynamic (in the sense that parts of it can be removed and garbage collected).

I suspect that this might be rather fundamental: once you participate in Python's MRO inheritance mechanism, you'd probably already get permanently nailed there. Python is definitely not designed with a non-monotonously growing class hierarchy in mind, so I expect that the caching and storing strategies put in place to implement it, prevent decreases (by "accidental design" I expect -- as a consequence of other design decisions made; not as an intentional design goal)

This was implicitly acknowledged in #15801.

We suffer from similar effects in the coercion network and the conversion caching: it's just very hard to keep a graph in such a way that you can efficiently find paths and force path independence of other properties along these paths (the coercion maps; unambiguous inheritance) and still allow for nodes to vanish from the graph as well - in particular if you'd like independence to hold through time (at least limited to a run) as well. [and another source -- caches that enforce global uniqueness :-)]

mkoeppe commented 3 years ago
comment:26

Setting a new milestone for this ticket based on a cursory review.