failed conversion yields unconclusive error message

[mjungmath]

At this stage, the conversion

sage: M = Manifold(2, 'M')
sage: M.diff_form_module(1)(1)
---------------------------------------------------------------------------
AttributeError                            Traceback (most recent call last)
...
AttributeError: 'NoneType' object has no attribute '_domain'

fails with an AttributeError.

This should rather yield a NotImplementedError or TypeError with the message that there is no conversion available.

CC: @egourgoulhon @tscrim @mkoeppe

Component: manifolds

Author: Michael Jung

Branch/Commit: 28dec69

Reviewer: Travis Scrimshaw, Eric Gourgoulhon

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

[mjungmath]

Description changed:

--- 
+++ 
@@ -10,4 +10,4 @@

fails with an AttributeError.

-This should rather yield a NotImplementedError with the message that there is no conversion available. +This should rather yield a NotImplementedError or TypeError with the message that there is no conversion available.

[mjungmath]

Branch: u/gh-mjungmath/conversion_attributeerror_fix

[mjungmath]

comment:3

Here we go. Let's see what our friend the patchbot says.

---

New commits:

1bbc830

Trac #30191: AttributeError catched and TypeError raised instead

[mjungmath]

Commit: 1bbc830

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

Changed commit from 1bbc830 to d577e16

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

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

66c2ca8	Trac #30191: integer check via ZZ + not convertible error if no list
d577e16	Trac #30191: conversion error for automorphism group

[mjungmath]

comment:5

This should be better. When the user inserts a list for comp, I guess we can assume that the user knows what he/she is doing.

Furthermore, I have replaced the isinstance(comp, (int, Integer)) checks with comp in ZZ all over the files. This is more appropriate and easier to read, I think.

Please let me know if I have overlooked something.

[mkoeppe]

comment:6

Replying to @mjungmath:

Let's see what our friend the patchbot says.

Patchbot does not like to talk about tickets without Authors

[mjungmath]

Author: Michael Jung

[mjungmath]

comment:7

Indeed. :D

[tscrim]

comment:8

Two things to be aware:

-        if comp:
+        if comp != []:

This will not only be slower, but more restrictive (it doesn't cover tuples). Is there some explicit reason why you are changing this? If so, you need to insert a comment about this as it is very likely that it will be removed later on.

Checking x in ZZ means x can be rational numbers like 2/1 and is slower than isinstance(x, (int, Integer)). The pollution of your data by rationals (and other others like SR elements, polynomial rings, etc.) masquerading as integers is possible, which might result in strange error messages later on, such as an AttributeError because it is expecting an Integer. While this may not happen, it is something you should be aware of.

[mjungmath]

comment:9

Replying to @tscrim:

Two things to be aware:

-        if comp:
+        if comp != []:

This will not only be slower, but more restrictive (it doesn't cover tuples). Is there some explicit reason why you are changing this? If so, you need to insert a comment about this as it is very likely that it will be removed later on.

I wanted to unify this. I agree, the if comp: version is much better. (Forget my original post. It was too early in the morning for me :D)

Checking x in ZZ means x can be rational numbers like 2/1 and is slower than isinstance(x, (int, Integer)). The pollution of your data by rationals (and other others like SR elements, polynomial rings, etc.) masquerading as integers is possible, which might result in strange error messages later on, such as an AttributeError because it is expecting an Integer. While this may not happen, it is something you should be aware of.

I see. For the conversion itself, I think the in ZZ is preferrable. Also, there is nothing to worry about.

For the other inputs, what you say sounds perfectly reasonable. I'll change this back.

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

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

fe43915

Trac #30191: code improvements + in ZZ check removed again

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

Changed commit from d577e16 to fe43915

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

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

461ff65

Trac #30191: != [] removal reverted

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

Changed commit from fe43915 to 461ff65

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

Changed commit from 461ff65 to af5af8f

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

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

af5af8f

Trac #30191: elif reverted + in ZZ check for automorphismfield_group

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

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

f0c6ee8

Trac #30191: minor fix

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

Changed commit from af5af8f to f0c6ee8

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

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

f1221da

Trac #30191: minor fix

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

Changed commit from f0c6ee8 to f1221da

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

Changed commit from f1221da to a042313

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

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

f0c6ee8	Trac #30191: minor fix
a042313	Trac# 30191: wrong indentation reverted

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

Changed commit from a042313 to 4cc92e9

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

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

4cc92e9

Trac #30191: comp check unified to if comp

[mjungmath]

comment:18

This should be it. Sorry for the mess. I really hate to work on many files at one time.

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

Changed commit from 4cc92e9 to f98a47b

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

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

f98a47b

Trac #30191: unused ScalarField import removed

[mjungmath]

comment:21

Patchbot is green. Awaiting approval. :)

[tscrim]

Reviewer: Travis Scrimshaw

[tscrim]

comment:22

I am not sure why we need to check that comp in ZZ and comp == 0 and not just let the coercion framework do the check. Basically, I don't know what having that extra first comp in ZZ is suppose to catch as equality testing should never result in an error. Other than that, LGTM.

[mjungmath]

comment:23

One first example that comes to my mind is having a zero in GF(p), or any other incompatible ring. Even though this is a zero, it is not an element compatible to a tensor field.

Secondly, I am not sure if all objects outside of the coercion framework, i.e. not even having an algebraic structure, always yield False for comp == 0.

[tscrim]

comment:24

Replying to @mjungmath:

One first example that comes to my mind is having a zero in GF(p), or any other incompatible ring. Even though this is a zero, it is not an element compatible to a tensor field.

You could either call it ducktyping or a conversion, but I don't see the point in this test. Also, there is this:

sage: GF(5).zero() in ZZ
True
sage: GF(5).zero() == 0
True

Secondly, I am not sure if all objects outside of the coercion framework, i.e. not even having an algebraic structure, always yield False for comp == 0.

All of the reasonable things (lists, tuples, sets, etc.) compare with False. If someone wants to put some garbage in, then they should expect garbage out (or breakage). So I wouldn't worry that.

[mjungmath]

comment:25

Replying to @tscrim:

Replying to @mjungmath:

One first example that comes to my mind is having a zero in GF(p), or any other incompatible ring. Even though this is a zero, it is not an element compatible to a tensor field.

You could either call it ducktyping or a conversion, but I don't see the point in this test. Also, there is this:

sage: GF(5).zero() in ZZ
True
sage: GF(5).zero() == 0
True

Interesting. This is not a mathematical rigorous behavior, is it?

Anyway, this seems hold true for the polynomial ring and the rational numbers, too. Then the in ZZ check is indeed redundant.

Secondly, I am not sure if all objects outside of the coercion framework, i.e. not even having an algebraic structure, always yield False for comp == 0.

All of the reasonable things (lists, tuples, sets, etc.) compare with False. If someone wants to put some garbage in, then they should expect garbage out (or breakage). So I wouldn't worry that.

Fair enough. :D

That was quite convincing. I will remove the in ZZ check.

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

Changed commit from f98a47b to c34e481

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

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

c34e481

Trac #30191: remove in ZZ check

[mjungmath]

comment:27

Let's wait for the patchbot's response.

[tscrim]

comment:28

If the patchbot comes back green, then you can set a positive review on my behalf.

[egourgoulhon]

comment:29

Something to keep in mind: when performing the change

-        if isinstance(comp, (int, Integer)) and comp == 0:
+        if comp == 0:

the new test can be much slower than the previous one whenever comp is a non trivial symbolic expression. This can alter significantly the performances if comp is susceptible to belong to SR, or more generally is an object whose comparison to 0 is costly.

[mjungmath]

comment:30

Replying to @egourgoulhon:

Something to keep in mind: when performing the change

-        if isinstance(comp, (int, Integer)) and comp == 0:
+        if comp == 0:

the new test can be much slower than the previous one whenever comp is a non trivial symbolic expression. This can alter significantly the performances if comp is susceptible to belong to SR, or more generally is an object whose comparison to 0 is costly.

What about checking these cases separately then? We could try a fast check with is_trivial_zero prior to the usual check. Meaning something like this:

-        if isinstance(comp, (int, Integer)) and comp == 0:
+        if hasattr(comp, 'is_trivial_zero'):
+            if comp.is_trivial_zero():
+                return self.zero()
+        elif comp == 0:
+            return self.zero()

This would cover more potential cases without decreasing the speed. Or is the hasattr check slow?

Addendum: Since hasattr just seems to catch the error, this one might be better:

-        if isinstance(comp, (int, Integer)) and comp == 0:
+        try:
+            if comp.is_trivial_zero():
+                return self.zero()
+        except AttributeError:
+            if comp == 0:
+                return self.zero()

[tscrim]

comment:31

Then you get a slowdown for that extra check, and catching an exception is more costly than checking hasattr, although if the attribute is there, then the try-except is faster. Although I guess that is faster than checking that the parent is SR. It all depends on what you think is most likely to happen in "most" cases. (Plus, some other object might have an is_trivial_zero attribute, maybe a scalar field, but I guess because of the semantics, there isn't a problem here.)

[mjungmath]

comment:32

Replying to @tscrim:

Then you get a slowdown for that extra check, and catching an exception is more costly than checking hasattr, although if the attribute is there, then the try-except is faster. Although I guess that is faster than checking that the parent is SR. It all depends on what you think is most likely to happen in "most" cases.

Mh. What do you think is the best option?

(Plus, some other object might have an is_trivial_zero attribute, maybe a scalar field, but I guess because of the semantics, there isn't a problem here.)

In that case, I'd say again: garbage to whom garbage is due.

[egourgoulhon]

comment:33

Replying to @mjungmath:

Replying to @tscrim:

Then you get a slowdown for that extra check, and catching an exception is more costly than checking hasattr, although if the attribute is there, then the try-except is faster. Although I guess that is faster than checking that the parent is SR. It all depends on what you think is most likely to happen in "most" cases.

Mh. What do you think is the best option?

I would vote for the try-except. In any case, using is_trivial_zero is a good idea, because if an object is generically slow in comparing to zero, one might expect that it is endowed with a is_trivial_zero method. For sure this is the case for SR elements.

[mjungmath]

comment:34

Replying to @egourgoulhon:

Replying to @mjungmath:

Replying to @tscrim:

Then you get a slowdown for that extra check, and catching an exception is more costly than checking hasattr, although if the attribute is there, then the try-except is faster. Although I guess that is faster than checking that the parent is SR. It all depends on what you think is most likely to happen in "most" cases.

Mh. What do you think is the best option?

I would vote for the try-except. In any case, using is_trivial_zero is a good idea, because if an object is generically slow in comparing to zero, one might expect that it is endowed with a is_trivial_zero method. For sure this is the case for SR elements.

Good. I also think that this is a good approach. It is less restrictive and still preserves, at least to some amount, the conversion speed.

In case there is no further objection or probably better approach, I would apply the discussed change. Agreed?

[tscrim]

comment:35

I have no objections. (Just to be clear, I did not have any objections with adding the check; I was just simply trying to state the consequences of it.)

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

Changed commit from c34e481 to acc407c

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

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

65dff3d	Trac #30191: Merge branch 'develop' into conversion_attributeerror_fix
acc407c	Trac #30191: try-except block for zero check

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

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

0d27b57

Trac #30191: unnecessary comment block removed

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

Changed commit from acc407c to 0d27b57