Coercion discovery fails to be transitive

[nbruin]

As found in #14711:comment:134, the following example shows that a combination of register_embedding and register_coercion can lead to a failure in transitivity for coercion discovery; also discussed on sage-devel:

class pA(Parent): pass
class pB(Parent): pass
class pC(Parent): pass

A=pA(); B=pB(); C=pC()

BtoA=Hom(B,A)(lambda x: A(x))
AtoC=Hom(A,C)(lambda x: C(x))
A.register_coercion(BtoA)
A.register_embedding(AtoC)

G=get_coercion_model()
G.discover_coercion(A,C) #finds AtoC
G.discover_coercion(B,A) #finds BtoA
G.discover_coercion(B,C) #does not find the composition of BtoA with AtoC

One workaround is simple: just don't use register_embedding. However, after #14711, there are different lifetime implications between using register_embedding and register_coercion, so this workaround might not be desirable.

Depends on #14711 Depends on #15329 Depends on #15331

CC: @simon-king-jena @robertwb @jpflori

Component: coercion

Work Issues: rebase (11 files conflicting)

Author: Simon King

Branch/Commit: u/SimonKing/ticket/15303 @ 12e8055

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

[simon-king-jena]

comment:43

I think the attached branch is a progress towards a coercion system with a cleaner model. Even if there would remain rough edges.

My strategy: I'll try to analyse the remaining recursion errors, since this is likely to point us to fundamental flows. In the best case, the analysis will result in a better (formulation of) the model.

Currently, I am studying the following very simple example:

sage: L.<i> = NumberField(x^2 + 1); L
Number Field in i with defining polynomial x^2 + 1
sage: K, from_K, to_K = L.change_generator(i/2 + 3)
<Recursion boom>

[simon-king-jena]

comment:44

Shorter:

sage: L.<i> = NumberField(x^2 + 1)
sage: K = NumberField(L(i/2+3).minpoly(), names=('i0',), embedding=L(i/2+3))

[simon-king-jena]

comment:45

I wonder: Do we really insist on making the example from the ticket description work?

There are typical parents (e.g., CC) that have numerous registered coerce embeddings from other parents (e.g., from number fields). If we try to find a coercion from P into CC, it would normally not help to first search for a coercion from P into a number field instead.

So, perhaps we could say that registered embeddings should be ignored in backtracking (as they are now). If one wants something like the example from the ticket description, one should not use .register_embedding(), but do something like this:

C.coerce_map_from(B)   # returns None, since we have no A yet
A.register_coercion(BtoA)
AtoC.register_as_coercion() # should increase the cache version number according to the Lemma
C.coerce_map_from(B)  # now returns a coercion via A, because the cached absence of a coercion
                      # will be ignored after the increment of the version number

Do you agree on this change of aim?

[nbruin]

comment:46

Replying to @simon-king-jena:

I wonder: Do we really insist on making the example from the ticket description work?

There are typical parents (e.g., CC) that have numerous registered coerce embeddings from other parents (e.g., from number fields). If we try to find a coercion from P into CC, it would normally not help to first search for a coercion from P into a number field instead.

Ah right, you're saying there are certain "almost universal terminal" objects that tend to have a very large in-degree. Doing backtracking from them would be a rather expensive operation (independent of whether it's breadth-first (cheapest-first is a better name) or depth-first). So we mandate that any paths that would involve such a map should be explicitly offered, so that backtracking is not necessary for discovering paths that use it. That could make sense. In order for such a model to be usable, you'd have to write clear documentation/helper routines to ensure that the right maps are put in. What if discovery is needed in the process of finding coercions between polynomial rings/function fields/algebraic groups over certain bases etc? Can we write down rules that make for a transparent and usable model?

So, perhaps we could say that registered embeddings should be ignored in backtracking (as they are now). If one wants something like the example from the ticket description, one should not use .register_embedding(), but do something like this:

C.coerce_map_from(B)   # returns None, since we have no A yet
A.register_coercion(BtoA)
AtoC.register_as_coercion() # should increase the cache version number according to the Lemma
C.coerce_map_from(B)  # now returns a coercion via A, because the cached absence of a coercion
                      # will be ignored after the increment of the version number

Hm, where does AtoC.register_as_coercion() fit in with the "only initialize coercions upon __init__ of the parent"?

Do you agree on this change of aim?

Well, I think what you're saying is that under certain conditions we require that the graph is fed more than just the backbone. Instead of relying on the coercion system to discover the full transitive closure, we mandate that the graph as offered should already be transitively closed in some aspects. If you can make those aspects clear and workable, I guess that could be a solution too.

[simon-king-jena]

comment:47

Hi!

First of all, note that the latest recursion errors refer to not-yet-committed code. It could be that by copy-and-pasting I somehow destroyed the backtracking algorithm by not properly marking a used arrow as not-to-be-tested-again. Thus the recursion.

Replying to @nbruin:

Hm, where does AtoC.register_as_coercion() fit in with the "only initialize coercions upon __init__ of the parent"?

Both AtoC.register_as_coercion() and A.register_embedding(AtoC) add an arrow to the coerce digraph after initialisation of C. With regard to the above lemma, one should only cache the absence of a coercion until an arrow is inserted with a previously existing codomain. Hence, in both cases we need to increase the cache version number.

[simon-king-jena]

comment:48

OUCH!!

I just found that discover_coerce_map_from did not mark arrows as "used" at all! Hence, there is (another) bug in our backtracking algorithm.

[simon-king-jena]

comment:49

In #12969, I fixed the backtracking algorithm for the discovery of actions. There, the problem was that the absence of an action was cached in the middle of backtracking, i.e., even though some paths were temporarily forbidden, so that an action was easily found after allowing the paths again.

In the case of coerce maps, the backtracking algorithm is not properly implemented either. Namely, paths that have already been visited are not marked as "forbidden". I think this is a severe bug. It is amazing that it didn't show up before. In any case, it must be implemented here, since otherwise we will hardly be able to add the feature that this ticket is about.

[simon-king-jena]

Changed work issues from Analyse recursion error to Implement backtracking properly

[simon-king-jena]

comment:50

Oops, I spoke too soon. #12969 was in fact about backtracking for coerce maps (not for actions), and forbidden paths are in fact declared (_register_pair(self, S,"coerce")) if discover_coerce_map_from() is called by coerce_map_from().

That said, I wonder if the whole registration business couldn't be done more efficiently. For example, the construction of coercions are thoroughly based on the comparison of parents by identity (this even holds for non-unique parents). Hence, _register_pair should be rewritten as well, so that only identity and not equality counts. That's for a different ticket.

[simon-king-jena]

comment:51

I think I have located the problem: If the pair X,Y already is registered and we do X.coerce_map_from(Y), then None should be returned (but of course not cached). But currently it is still attempted to find a coercion from Y to X, resulting in an infinite loop.

[simon-king-jena]

comment:52

As it turns out, the recursion again occurs when calling (not when constructing!) a coerce map. Nevertheless, I think one should speed-up the backtracking based on comparison by identity. Perhaps one could even use a TripleDict for storage of temporarily forbidden paths.

The problematic map is a DefaultConvertMap_unique, and this could explain why the problem arose now: In vanilla Sage, this kind of maps is only returned if the backtracking finds nothing better.

But in one of my commits, I made sure that the backtracking is not attempted if self._coerce_map_from_(S) returns True or returns an actual map. The rationale for this change was that self._coerce_map_from_(S) is an answer given by a specific object and should just be preferred over an answer given by a general backtracking algorithm. As it turns out, Sage relies on the assumption that backtracking usually is better.

In other words: In vanilla Sage, dependencies between arrows of the coerce digraph are implicitly declared by

• choosing an order of registered coercions
• avoiding DefaultConverMap and friends.

I am not happy about the existing way of implicit declaration of dependencies and think we should do better!

[simon-king-jena]

comment:53

I think the following is both reasonable and reasonably close to the status quo:

• if _coerce_map_from_ returns a map, then we trust that it is a good choice.
• if _coerce_map_from_ returns True, then we do not necessarily trust that self._generic_convert_map(S) is the best choice. So, we test if backtracking yields anything better.

I found a couple of cases in which _coerce_map_from_ returned _generic_convert_map (e.g. for CC!). It seems to me that one should return True instead.

[nbruin]

comment:54

Replying to @simon-king-jena:

I think the following is both reasonable and reasonably close to the status quo:

• if _coerce_map_from_ returns a map, then we trust that it is a good choice.
• if _coerce_map_from_ returns True, then we do not necessarily trust that self._generic_convert_map(S) is the best choice. So, we test if backtracking yields anything better.

Can we explain these rules purely in terms of the coercion graph? Does _coerce_map_from_(..)==True somehow indicate an edge with a property (colour?) that should be avoided or an edge with the cost modified (increased)? If the graph version is increased, do we invalidate paths with a bad colour as well, because now there might be one with a better colour? (OK, we can't call this property colour, obviously. That's just going to be too politically incorrect).

My hunch is that the undesirability of a certain edge can simply be expressed in its cost and that we only need a 1-dimensional cost parameter to capture all that we need [we'd have to think about calibration, though].

Above, you also talk about "forbidden" and "temporarily forbidden" paths. Are those part of the depth-first path discovery? That would be an even better reason to see if we can get a "cheapest first" search in, since that naturally avoids cycles.

[simon-king-jena]

comment:55

Good(?) news: I can track down the error to the following in vanilla Sage.

sage: L.<i> = NumberField(x^2 + 1)
sage: K = NumberField(L(i/2+3).minpoly(), names=('i0',), embedding=L(i/2+3))
sage: from sage.rings.number_field import number_field_morphisms
sage: number_field_morphisms.EmbeddedNumberFieldMorphism(R, self)
Traceback (most recent call last):
...
RuntimeError: maximum recursion depth exceeded in __instancecheck__

The point is that the with my patch, the above code is executed during construction of K, in order to internally construct something. I'll ask on sage-nt whether this code should give a map. I guess it should not, but perhaps the number theorists disagree. And if it should not give a map, then there should be a TypeError (or ValueError) raised, without infinite recursion.

[jpflori]

comment:57

Great work, all of this looks really fine.

[simon-king-jena]

comment:58

Replying to @nbruin:

Above, you also talk about "forbidden" and "temporarily forbidden" paths. Are those part of the depth-first path discovery?

Of course. By "temporarily forbidden", I mean those that have already been visited in the depth-first search and shall thus not be visited again during the same search. Thus "temporarily", because in the next search they could be visited.

That would be an even better reason to see if we can get a "cheapest first" search in, since that naturally avoids cycles.

How would that be?

Also note that if you want to compare costs, you need to construct the maps. It could become very costly, if you want to compare many paths.

Replying to @simon-king-jena: Can we explain these rules purely in terms of the coercion graph?

Somehow (see below).

Does _coerce_map_from_(..)==True somehow indicate an edge with a property (colour?) that should be avoided or an edge with the cost modified (increased)?

Not at all. _coerce_map_from_ is only called if there is no edge. Note that "long time ago" (in one of the posts above) I talked about "short-cuts" in the coerce digraph. _coerce_map_from_ is for the shortcuts, not for the edges.

So, in terms of the coerce digraph: We have a digraph with some connected components X,Y. It is possible that there is a coercion from a node x in X to a node y in Y; this is the case of y._coerce_map_from_(x) returns True or returns a map. However, this does not mean that an edge is inserted that would connect X and Y: The two components remain separate, and a reference to any node in Y does not prevent the whole component X from garbage collection.

This situation is easy. Difficulties only arise if x and y belong to the same component X. In this situation, we may have several possibilities to construct a map from x to y: There could be different paths in X, and there could be a map provided by y._coerce_map_from_(x), or y._coerce_map_from_(x) returns True. In the latter case, it means that we can use y._generic_convert_map(x) for coercion.

Which of these maps to choose? It is required that these maps are mathematically the same. So, our choice can not be based on the maths only.

[nbruin]

comment:59

Replying to @simon-king-jena:

Also note that if you want to compare costs, you need to construct the maps. It could become very costly, if you want to compare many paths.

Well, you could hold off on actually creating the map composition until you've determined the lowest cost path. However, I think we have bigger problems to solve before we could contemplate doing this.

So, in terms of the coerce digraph: We have a digraph with some connected components X,Y. It is possible that there is a coercion from a node x in X to a node y in Y; this is the case of y._coerce_map_from_(x) returns True or returns a map. However, this does not mean that an edge is inserted that would connect X and Y: The two components remain separate, and a reference to any node in Y does not prevent the whole component X from garbage collection.

This situation is easy. Difficulties only arise if x and y belong to the same component X. In this situation, we may have several possibilities to construct a map from x to y: There could be different paths in X, and there could be a map provided by y._coerce_map_from_(x), or y._coerce_map_from_(x) returns True. In the latter case, it means that we can use y._generic_convert_map(x) for coercion.

(nitpick: I don't think we want to talk about "connected components" if we're considering a digraph. Perhaps we should say y is "reachable" from x).

If y._coerce_map_from_(x) can return "true" or a map if the coercion graph otherwise doesn't have a path from x to y then the current coercion model is significantly different from the digraph model we're talking about. If we have a coercion path X->Z->Y where the coercion from Y to Z can only be found using _coerce_map_from_ then we cannot discover the coercion from X to Y (how would we know to involve Z?) but we can discover the coercions X->Z and Z->Y. So we're fundamentally breaking transitivity already.

Perhaps one way out is if _coerce_map_from_ indeed only returns "shortcuts": paths between nodes that exist otherwise, so only in the situation you describe as "Difficult".

If we cannot trust that the return map from _coerce_map_from_ (either explicitly, or the implied conversion) is the best possible, then there's no use in having it return anything: we'd have to investigate the rest of the coercion graph anyway.

I agree that relying on conversion maps in the coercion framework looks very suspect: conversions are supposed to be trying coercion first!

So it seems to me that _coerce_map_from_ returning True should perhaps be abolished entirely and that in other cases one should be very careful with implementing _coerce_mapfrom. It can certainly not be the only coercion hook one provides if we want to have a digraph model.

[nbruin]

comment:60

Replying to @simon-king-jena:

Good(?) news: I can track down the error to the following in vanilla Sage.

sage: L.<i> = NumberField(x^2 + 1)
sage: K = NumberField(L(i/2+3).minpoly(), names=('i0',), embedding=L(i/2+3))
sage: from sage.rings.number_field import number_field_morphisms
sage: number_field_morphisms.EmbeddedNumberFieldMorphism(R, self)
Traceback (most recent call last):
...
RuntimeError: maximum recursion depth exceeded in __instancecheck__

The point is that the with my patch, the above code is executed during construction of K, in order to internally construct something. I'll ask on sage-nt whether this code should give a map. I guess it should not, but perhaps the number theorists disagree. And if it should not give a map, then there should be a TypeError (or ValueError) raised, without infinite recursion.

I suppose you mean

sage: number_field_morphisms.EmbeddedNumberFieldMorphism(K,L)

According to the documentation it shouldn't because by default it tries to see if K and L are both naturally embedded in CC, and they are not.

The following already does work:

sage: number_field_morphisms.EmbeddedNumberFieldMorphism(K,L,L) #any combination of K and L
Generic morphism:
  From: Number Field in i0 with defining polynomial x^2 - 6*x + 37/4
  To:   Number Field in i with defining polynomial x^2 + 1
  Defn: i0 -> 1/2*i + 3
sage: number_field_morphisms.EmbeddedNumberFieldMorphism(L,L)
Generic endomorphism of Number Field in i with defining polynomial x^2 + 1
  Defn: i -> i

The latter suggests that the routine does take some liberties. For K we get

sage: number_field_morphisms.EmbeddedNumberFieldMorphism(K,K)
RuntimeError: maximum recursion depth exceeded while calling a Python object

so the difference in behaviour between L and K seems dodgy.

[simon-king-jena]

comment:61

Replying to @nbruin:

Well, you could hold off on actually creating the map composition until you've determined the lowest cost path.

No, because K._coerce_map_from_(L) gives you shortcuts. It may very well be that it returns a map that is much "cheaper" then a composition of registered coercions and registered embeddings leading from L to K.

And note that these shortcuts will also arise during backtracking. Namely, if K has registered coercions from R1,...,Rn, then the backtracking will involve to ask R1._coerce_map_from_(L), ..., Rn._coerce_map_from_(L) for shortcuts.

Hence, it is not enough to assign costs to arrows in the digraph.

(nitpick: I don't think we want to talk about "connected components" if we're considering a digraph. Perhaps we should say y is "reachable" from x).

Correct.

If y._coerce_map_from_(x) can return "true" or a map if the coercion graph otherwise doesn't have a path from x to y then the current coercion model is significantly different from the digraph model we're talking about.

No. This is exactly the digraph-with-shortcuts model I kept talking about since comment 84 of #14711 (hence, since 3 weeks). Since then, I used the term "digraph model" only as an abbreviation of the lengthy term "digraph-with-shortcuts model".

If we have a coercion path X->Z->Y where the coercion from Y to Z can only be found using _coerce_map_from_

You mean from Z to Y?

then we cannot discover the coercion from X to Y (how would we know to involve Z?) but we can discover the coercions X->Z and Z->Y. So we're fundamentally breaking transitivity already.

Yes, which is part of the specification of the digraph-with-shortcuts model.

That said: We now have version numbers of the coerce digraph. Hence, caching the absence of a coercion would no longer be a problem, and I could imagine that by now it would be possible to turn a short-cut Z->Y into an actual arrow of the digraph.

However, I could also imagine that this would have two disadvantages:

1. It would depend on the command history whether Sage would find a coercion from X to Y (via Z) or not. Arguably, it would be better than the status quo, which is "we won't find a coercion from X to Y via Z, even if we know that Z exists and has a coercion into Y".
2. The list of maps-to-be-considered in the backtracking algorithm grew. Hence, it would increase the running time for detecting a coerce map.

Perhaps one way out is if _coerce_map_from_ indeed only returns "shortcuts": paths between nodes that exist otherwise, so only in the situation you describe as "Difficult".

I don't understand what you mean by this.

In vanilla Sage, _coerce_map_from_ does return nothing but "shortcuts" (i.e., things that the backtracking algorithm does not know about). It does so both in the easy situation and in the difficult situation. The easy situation is if the backtracking algorithm finds no path, hence, the shortcut is the only option, hence, we don't have the burden of choosing something (therefore the situation is "easy").

If we cannot trust that the return map from _coerce_map_from_ (either explicitly, or the implied conversion) is the best possible, then there's no use in having it return anything: we'd have to investigate the rest of the coercion graph anyway.

I think I have already described what is currently done:

1. It is tested whether there is a short-cut phi (either explicitly or implicitly).
2. Even if phi is found, backtracking is performed. Backtracking "goes back" according to the registered coercions, and may involve short-cuts by recursion. Let's call psi the first map found by backtracking (if there is any).
3. If both phi and psi are available, then return the less costly. If only one of them is available, return it. Otherwise, there is no coercion.

My current commits (at least those I have at home) change it as follows

• If _coerce_map_from_ explicitly returns an actual map, then the backtracking search is not performed, but it is trusted that the map is good to be used.
• If _coerce_map_from_ returns True, then it says implicitly that _generic_convert_map could in principle be used for coercion, but there might be better options. Hence, similar to vanilla Sage, my branch would do a backtracking and return the less costly map.
• Registered embeddings are used for backtracking, too, whereas vanilla Sage only uses registered coercions.

We might play with the idea of extending the last point, namely: Turn the short-cuts into permanent arrows, to be used by backtracking. But then we need to take care of lifetime implications, and we need to take care of a potential slow-down of the backtracking.

I agree that relying on conversion maps in the coercion framework looks very suspect: conversions are supposed to be trying coercion first!

I did not state that it looks suspect. Hence, we don't agree. Saying "we use conversion as coercion" is just short for "we take the map that is returned by _generic_convert_map, cache this map in _coerce_from_cache, and henceforth use this map for coercion (and of course for conversion, too)".

So it seems to me that _coerce_map_from_ returning True should perhaps be abolished entirely and that in other cases one should be very careful with implementing _coerce_mapfrom.

I disagree.

It can certainly not be the only coercion hook one provides if we want to have a digraph model.

If we want to keep the current digraph-with-shortcuts model, then we indeed need at least two hooks: One that adds arrows to the digraph, and one that provides short-cuts. In my branch, I suggest to extend the role of registered embeddings, namely: To make them actual arrows, to be used in backtracking.

[nbruin]

comment:62

Replying to @simon-king-jena:

No, because K._coerce_map_from_(L) gives you shortcuts. It may very well be that it returns a map that is much "cheaper" then a composition of registered coercions and registered embeddings leading from L to K.

So shouldn't the rule be: If K._coerce_map_from_(L) returns something then one of the following must be true

• There is a registered coercion from L to K
• There is a registered embedding from L to K
• For any structure M that coerces into L, K._coerce_mapfrom(M) also returns something.

i.e., either the coercion system has enough information to compute transitive closures by itself or _coerce_map_from_ ensures transitive closure by itself already.

Hence, it is not enough to assign costs to arrows in the digraph.

No, "shortcut" paths returned by _coerce_map_from_ should carry a cost as well, obviously. And one would hope their cost roughly corresponds the cheapest path that can be discovered otherwise (perhaps with a small modifier to make it more attractive to use this particular path).

Aren't the "shortcuts" supposed to be an implementation detail to more efficiently implement the theoretical digraph model? In that case we can reason about their desired properties purely based on the theoretical digraph model.

If you're claiming the shortcuts are something else and deserve their own theoretical interpretation as well, I'd be interested in seeing what you mean by them.

No. This is exactly the digraph-with-shortcuts model I kept talking about since comment 84 of #14711 (hence, since 3 weeks). Since then, I used the term "digraph model" only as an abbreviation of the lengthy term "digraph-with-shortcuts model".

This makes me wonder what exactly the axioms of this "digraph-with-shortcuts model" is. I can see how "digraph-with-shortcuts" can be used to implement a proper digraph model more efficiently, if the shortcuts follow some rules (as stated above). If our theoretical model is "digraph-with-costs" I can see a "digraph-with-costs-and-shortcuts" implementation, where the shortcuts would obviously have to provide a cost on them too: the cost of the path they are providing the "shortcut" for (modulo little tweaks).

You mean from Z to Y?

Yes, sorry.

Yes, which is part of the specification of the digraph-with-shortcuts model.

My conclusion would be that our digraph-with-shortcuts implementation is failing to properly implement a straight digraph model.

That said: We now have version numbers of the coerce digraph. Hence, caching the absence of a coercion would no longer be a problem, and I could imagine that by now it would be possible to turn a short-cut Z->Y into an actual arrow of the digraph.

No, that's going to be a mess. I think the "actual arrow" would basically have to be there already (or be implied to be there by _coerce_map_from_ somehow magically ensuring that it will be returning results consistent with it).

Perhaps one way out is if _coerce_map_from_ indeed only returns "shortcuts": paths between nodes that exist otherwise, so only in the situation you describe as "Difficult".

I don't understand what you mean by this.

Hopefully my explanation above now clarifies this: In my opinion "-with-shortcuts" is an implementation detail to gain efficiency, not a part of the model, because I don't see what kind of reasonable model one would have then.

I think I have already described what is currently done:

1. It is tested whether there is a short-cut phi (either explicitly or implicitly).
2. Even if phi is found, backtracking is performed. Backtracking "goes back" according to the registered coercions, and may involve short-cuts by recursion. Let's call psi the first map found by backtracking (if there is any).
3. If both phi and psi are available, then return the less costly. If only one of them is available, return it. Otherwise, there is no coercion.

It seems to me they are not just "shortcuts" then. It seems that an opportunity is provided to provide parts of the graph in a programmatic way. As pointed out, such a presentation is not transparent for backtracking, so if one wants to provide something that is closed under path composition, we would need that the implemented _coerce_map_from_ ensures the closure by itself.

That means that providing coercions (just) via _coerce_map_from_ is actually a rather difficult thing to do correctly, whereas presently I think we advertise it as the easier, almost preferred way of doing it.

My current commits (at least those I have at home) change it as follows

• If _coerce_map_from_ explicitly returns an actual map, then the backtracking search is not performed, but it is trusted that the map is good to be used.

I think that's a reasonable thing to do.

• If _coerce_map_from_ returns True, then it says implicitly that _generic_convert_map could in principle be used for coercion, but there might be better options.

A feature like that might be OK (I guess it makes it easier to reuse code, because the programming paradigm makes it a little easier to implement "conversions" (parent call) than to implement coercions directly, but "correct" use would need that map, or an equivalent one, to be available for backtracking as well, or above-described closed property of _coerce_map_from_.

So it seems to me that _coerce_map_from_ returning True should perhaps be abolished entirely and that in other cases one should be very careful with implementing _coerce_mapfrom.

I disagree.

What other purpose does _coerce_map_from_ serve than shortcutting a backtracking search? If that is its only purpose then getting a result back that may compare unfavourably with alternative results from backtracking is useless. We may have the resulting _generic_convert_map as a registered coercion then, so that it naturally gets considered as part of the normal search.

OK, above I think we have identified that _coerce_map_from_ serve another purpose: Providing paths into a parent programmatically, leaving transitive closure properties as a responsibility to the implementor of the specific _coerce_map_from_ routine.

Conjecture: (Robert may be able to confirm or deny) The coercion model was designed to implement a digraph. However, the costs of full recursive backtracking were feared to be too high, so it was hoped that in a lot of cases, paths could be computed rather than recursively discovered, leaving only a relatively small number of candidates to be compared (i.e., Y._coerce_map_from_(X) should usually return a map directly, if there is one at all, so backtracking from Z shouldn't normally have to search very far to find a Y with a registered coercion into Z that knows how to handle X).

The cost in this approach is that _coerce_map_from_ needs to handle transitive closure by itself already. Backing it up with registered coercions anyway would leave us with too many paths to backtrack on anyway [that's only slightly mitigated by your "early exit" proposal]

[simon-king-jena]

comment:63

Replying to @nbruin:

So shouldn't the rule be: If K._coerce_map_from_(L) returns something then one of the following must be true

• There is a registered coercion from L to K
• There is a registered embedding from L to K

Better: A sequence of registered embeddings and coercions.

• For any structure M that coerces into L, K._coerce_mapfrom(M) also returns something.

I think so.

Hence, it is not enough to assign costs to arrows in the digraph.

No, "shortcut" paths returned by _coerce_map_from_ should carry a cost as well, obviously. And one would hope their cost roughly corresponds the cheapest path that can be discovered otherwise (perhaps with a small modifier to make it more attractive to use this particular path).

Well, the point I wanted to make is: One needs to construct the maps, because only the map know about their costs.

Aren't the "shortcuts" supposed to be an implementation detail to more efficiently implement the theoretical digraph model? In that case we can reason about their desired properties purely based on the theoretical digraph model.

Perhaps. But it may also affect lifetime implications.

[nbruin]

comment:64

Replying to @simon-king-jena:

Well, the point I wanted to make is: One needs to construct the maps, because only the map know about their costs.

Don't we almost always have the map in hand? When we find the map as a registered coercion or embedding, we can readily read off the cost. We can also do that when we get a map back from _coerce_map_from_. The only case where we have to estimate the cost is when we get the existence of a map via K._coerce_map_from_(L)==True, but in that case I expect the cost is a fixed number anyway, because it's a generically constructed map.

I assume that the cost of a composition is the sum of the costs of the composed maps, because otherwise we are not working on a weighted digraph.

That means we can simply sum the costs of the edges to get the cost of a path. No need to compute the "composition" of the maps explicitly until we actually need it.

Your "don't consider other maps coming into K if we're getting an actual map back from K._coerce_map_from_(L)" is then simply an assumption that K._coerce_map_from_(L) returns the cheapest map available. I think it helps making such assumptions about the coercion discovery algorithm explicit.

[nbruin]

comment:65

Replying to @simon-king-jena:

I think the following is both reasonable and reasonably close to the status quo:

• if _coerce_map_from_ returns a map, then we trust that it is a good choice.
• if _coerce_map_from_ returns True, then we do not necessarily trust that self._generic_convert_map(S) is the best choice. So, we test if backtracking yields anything better.

Unfortunately, that doesn't seem to be a very good heuristic because it's not stable: _coerce_map_from_ is supposed to do some backtracking on its own, so if A._coerce_map_from(B) happens to consider another parent C that coerces into A and find that C._coerce_map_from(B)==True, it would return an explicit composite map, with the generic conversion from B into C as one of the components. This map would receive preference, but this map should be even less attractive because on top of a generic conversion, it is also composed with some other map.

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

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

[changeset:5c0800a]	_coerce_map_from_ shouldn't return default conversion maps. Fix for embedded number field morphisms
[changeset:b7fe7fc]	Use registered coercions *and* embeddings for backtracking, but prefer the former.
[changeset:a18e13e]	Increase coerce cache version in Morphism.register_as_coercion()

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

Changed commit from 74821fe to 5c0800a

[simon-king-jena]

Changed work issues from Implement backtracking properly to none

[simon-king-jena]

comment:67

Replying to @nbruin:

Unfortunately, that doesn't seem to be a very good heuristic because it's not stable: _coerce_map_from_ is supposed to do some backtracking on its own, so if A._coerce_map_from(B) happens to consider another parent C that coerces into A and find that C._coerce_map_from(B)==True, it would return an explicit composite map, with the generic conversion from B into C as one of the components. This map would receive preference, but this map should be even less attractive because on top of a generic conversion, it is also composed with some other map.

Why should A._coerce_map_from(B) return anything? Aren't you rather talking about A.discover_coerce_map_from(B) that recurses to C and thus calls C._coerce_map_from_(B)?

In any case, I have now pushed my recent commits. I did solve the recursion error with embedded number field morphism (solution: Raise a TypeError if one of the number fields actually is not embedded), and with this change

sage: L.<i> = NumberField(x^2 + 1)
sage: K = NumberField(L(i/2+3).minpoly(), names=('i0',), embedding=L(i/2+3))

works like a charm. I don't know yet whether it results in other problems, but in any case I'll revert to "needs review" now.

[simon-king-jena]

comment:68

PS: It might be a good idea to get some number theorist to look at the change to EmbeddedNumberFieldMorphism and confirm whether this change makes sense for the number theoretical applications.

[simon-king-jena]

Work Issues: Crash in permgroup.py

[nbruin]

comment:70

Replying to @simon-king-jena:

Why should A._coerce_map_from(B) return anything?

Because if I'm not mistaken, the implementor of A can decide to implement coercion into A via _coerce_mapfrom. Thus we'd have

class type_of_A(...):
...
    def _coerce_map_from_(self,S):
        if S is self.C_on_which_A_is_based:
            return True
        return self._coerce_map_via([self.C_on_which_A_is_based],S)

class type_of_C(...):
...
    def _coerce_map_from_(self,S):
        if is_instance(S,type_of_B):
            return True
        <other stuff>

With this one would get

sage: A._coerce_map_from_(C)
True
sage: C._coerce_map_from_(B)
True
sage: A._coerce_map_from_(B)
Composite map ...

As far as I can see (taking, for instance CC._coerce_map_from_ as an example) this would be a legitimate way of implementing coercion, and as you can see, the map found from C to A is explicitly returned, but consists of maps that are generic conversion maps.

Aren't you rather talking about A.discover_coerce_map_from(B) that recurses to C and thus calls C._coerce_map_from_(B)?

No, I'm talking about implementing part of the coercion graph (with backtracking on its paths) programmatically as part of the implementation of a parent via _coerce_map_from_. I think this was expressly a part of the current design and I think there are many examples of this in sage.

Reading the documentation of discover_coerce_map_from, it seems that routine would have the same issue (because it also seems to dislike DefaultConvertMap, and the fragment above shows that it is quite possible to get a composition of DefaultConvertMaps back, which should be even more disliked)

At this point it seems to me the coercion graphs gets specified in two ways:

• the _coerce_map_from_ world, where coercions and backtracking procedures are provided in a distributed, programmatic fashion. One would hope that the paths returned by this bit are consistent with what you'd get from finding paths on an actual graph.
• the _populate_coercion_lists_ world, which more readily corresponds to a graph presentation.

The total coercion graph is supposed to be the union of the two, but the way in which the merger is done seems murky and extremely implementation-dependent to me at the moment (perhaps because the _coerce_map_from_ world is completely implementation dependent by design).

I think we'd need to clarify the relation between the two a little more, and hopefully formulate some axioms that we can assume the _coerce_map_from_ world is adhering to before it's wise to further complicate the situation by also implementing backtracking along embeddings.

I think the investigations here are very useful, because I think we are making explicit a lot of implicit facts and assumptions that have been floating around in the coercion system and probably got violated in the organic evolution of the system.

[simon-king-jena]

comment:71

These are the errors one is getting with the current commits:

sage -t src/sage/groups/perm_gps/permgroup.py  # Killed due to abort
sage -t src/sage/combinat/sf/sfa.py  # 1 doctest failed
sage -t src/sage/rings/number_field/number_field.py  # 11 doctests failed
sage -t src/sage/combinat/sf/new_kschur.py  # 2 doctests failed
sage -t src/sage/tests/book_schilling_zabrocki_kschur_primer.py  # 1 doctest failed
sage -t src/sage/combinat/root_system/weyl_characters.py  # 1 doctest failed
sage -t src/sage/combinat/sf/sf.py  # 2 doctests failed
sage -t src/sage/structure/parent.pyx  # 1 doctest failed
sage -t src/sage/rings/residue_field.pyx  # 1 doctest failed
sage -t src/sage/combinat/symmetric_group_algebra.py  # 1 doctest failed
sage -t src/sage/combinat/ncsf_qsym/ncsf.py  # 4 doctests failed
sage -t src/sage/rings/polynomial/multi_polynomial_libsingular.pyx  # 1 doctest failed
sage -t src/sage/rings/number_field/number_field_morphisms.pyx  # 1 doctest failed
sage -t src/sage/rings/universal_cyclotomic_field/universal_cyclotomic_field.py  # 13 doctests failed
sage -t src/sage/rings/fraction_field_FpT.pyx  # 2 doctests failed
sage -t src/sage/categories/algebras.py  # 1 doctest failed
sage -t src/sage/categories/with_realizations.py  # 1 doctest failed
sage -t src/sage/rings/padics/padic_base_coercion.pyx  # 3 doctests failed

It is perhaps not as bad as it looks. Some of these errors come from weakening of coerce maps, namely: Some of the coerce maps used in doctests were not weakened by #14711, but became weakened now. So, that's trivial to fix.

What is bad is (at least) the crash in permgroup.py, and 11 errors on number fields also sounds like too many...

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

Changed commit from 5c0800a to 8d3caea

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

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

[changeset:8d3caea]

Copy some coerce maps in doctests. Use default conversion to coerce ZZ into QQbar

[simon-king-jena]

comment:73

I have just pushed a new commit that should fix most of the trivial problems. But not all is good, I guess

---

New commits:

[changeset:8d3caea]

Copy some coerce maps in doctests. Use default conversion to coerce ZZ into QQbar

[simon-king-jena]

comment:74

I detected another bug in vanilla Sage, that becomes a problem with my commits:

sage: from sage.categories.pushout import pushout
sage: pushout(Qp(7),RLF)
Traceback (most recent call last):
...
RuntimeError: maximum recursion depth exceeded while calling a Python object

[simon-king-jena]

comment:75

PS: I believe that fixing above bug boils down to re-thinking how the CompletionFunctor construction functors merge. Namely, both RLF and Qp(7) are constructed by completion of QQ.

[simon-king-jena]

comment:76

PPS: ... how they merge or how they commute!

Namely, the different completion functors apparently don't merge, but they commute, and I guess I am to blame for it. I guess completion functors should only commute if they complete at the same point. E.g., completion at 7 with precision 20 and completion at 7 with precision 40 does commute. But completion at 7 and completion at +Infinity should not commute.

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

Changed commit from 8d3caea to f8f79b8

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

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

[changeset:f8f79b8]

Fix commutation of CompletionFunctor. Further fix for embedded number field morphisms

[simon-king-jena]

comment:78

The new commit changes CompletionFunctor.commutes(..), so that the completion functor only commutes with the FractionField, but not with other CompletionFunctors (this is because other completion functors have already been taken care of by CompletionFunctor.merge(..)).

Also, it changes _coerce_map_from_ of number fields according to the previous changes in EmbeddedNumberFieldMorphism.

With the current commit, all tests in sage/rings/number_field/ pass.

[simon-king-jena]

Crash log

[simon-king-jena]

comment:79

Attachment: sage_crash_SqPXq6.log

I have attached the crash log that results from the permutation_group tests.

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

Changed commit from f8f79b8 to b9ebe81

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

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

[changeset:b9ebe81]

Fix some tests by taking *copies* of coerce maps

[simon-king-jena]

comment:81

I have pushed another commit. It fixes all remaining doctest errors, except the crash in permgroup.py. The fixes are rather easy: It was needed to add "..." to some expected error messages (reason: The changed string representation of weakened maps) and use non-weakened copies of some coerce maps. Apparently #14711 has not weakened all coercions.

Now, we can focus on the crash. No idea yet what is happening. Since it is an actual crash and not just an error that may be caught, I could imagine that some coercion is requested before relevant C data are provided.

---

New commits:

[changeset:b9ebe81]

Fix some tests by taking *copies* of coerce maps

[simon-king-jena]

comment:82

I would like to open two separate tickets for the two independent problems I mentioned. However, I was shooting myself into the foot: There are two relevant commits. The first commit deals with the first problem and with stuff that clearly belongs to here and not to a new ticket. The second commit deals with both the first and the second problem.

Anyway. I will try to rewrite the branch of this ticket and split off the two independent problems. Perhaps I will even try to "fold patches". Git certainly has the potential to let me do these changes.

Of course this means to change the history of the branch. But I strongly believe that only the net result of a ticket must matter. If a change in the history of a trac branch (without to change the resulting code!!!) causes problems for other trac branches that build on top of it, then this should be considered a bug in the current workflow. I will ignore this bug.

[simon-king-jena]

Changed work issues from Crash in permgroup.py to Crash in permgroup.py; rebase

[simon-king-jena]

comment:83

FWIW, I have created #15329 and #15331 to deal with the two independent sub-problems. Both need review.

[simon-king-jena]

Changed dependencies from #14711 to #14711 #15329 #15331

[simon-king-jena]

Last 10 new commits:

[changeset:807550b]	Fix some tests by taking *copies* of coerce maps
[changeset:1db6444]	Copy some coerce maps in doctests. Use default conversion to coerce ZZ into QQbar
[changeset:810fad8]	_coerce_map_from_ shouldn't return default conversion maps.
[changeset:d46cb9c]	Use registered coercions *and* embeddings for backtracking, but prefer the former.
[changeset:a04b480]	Increase coerce cache version in Morphism.register_as_coercion()
[changeset:1beac71]	Give user-provided coerce maps the same priority than to a coerce map found by backtracking
[changeset:bae64c4]	Store a version number for the coercion cache, depending on the registered embeddings
[changeset:7385549]	Use registered embeddings for backtracking in the coercion model
[changeset:40c787d]	Merge branch 'ticket/15331' into ticket/15303
[changeset:29ab8ee]	Merge branch 'ticket/15329' into ticket/15303