Opened 3 months ago

Closed 3 months ago

Last modified 3 months ago

#30191 closed defect (fixed)

failed conversion yields unconclusive error message

Reported by: gh-mjungmath Owned by:
Priority: major Milestone: sage-9.2
Component: manifolds Keywords:
Cc: egourgoulhon, tscrim, mkoeppe Merged in:
Authors: Michael Jung Reviewers: Travis Scrimshaw, Eric Gourgoulhon
Report Upstream: N/A Work issues:
Branch: 28dec69 (Commits) Commit: 28dec692737c1ce7e9fadb753ff3b8b1c378d4c2
Dependencies: Stopgaps:

Description (last modified by gh-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.

Change History (55)

comment:1 Changed 3 months ago by gh-mjungmath

  • Description modified (diff)

comment:2 Changed 3 months ago by gh-mjungmath

  • Branch set to u/gh-mjungmath/conversion_attributeerror_fix

comment:3 follow-up: Changed 3 months ago by gh-mjungmath

  • Commit set to 1bbc8306a46da97fdcf9ed82bdcf6bc1cf0dec74
  • Status changed from new to needs_review

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


New commits:

1bbc830Trac #30191: AttributeError catched and TypeError raised instead

comment:4 Changed 3 months ago by git

  • Commit changed from 1bbc8306a46da97fdcf9ed82bdcf6bc1cf0dec74 to d577e16f3ec58365235d20fa387ed23785a0d693

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

66c2ca8Trac #30191: integer check via ZZ + not convertible error if no list
d577e16Trac #30191: conversion error for automorphism group

comment:5 Changed 3 months ago by gh-mjungmath

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.

comment:6 in reply to: ↑ 3 Changed 3 months ago by mkoeppe

Replying to gh-mjungmath:

Let's see what our friend the patchbot says.

Patchbot does not like to talk about tickets without Authors

comment:7 Changed 3 months ago by gh-mjungmath

  • Authors set to Michael Jung

Indeed. :D

comment:8 follow-up: Changed 3 months ago by 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.

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.

comment:9 in reply to: ↑ 8 Changed 3 months ago by gh-mjungmath

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.

Last edited 3 months ago by gh-mjungmath (previous) (diff)

comment:10 Changed 3 months ago by git

  • Commit changed from d577e16f3ec58365235d20fa387ed23785a0d693 to fe4391559ee7d34dede77b532c1293be1a25b0b2

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

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

comment:11 Changed 3 months ago by git

  • Commit changed from fe4391559ee7d34dede77b532c1293be1a25b0b2 to 461ff656bc938720022b0bebdaef8ac1511916d0

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

461ff65Trac #30191: != [] removal reverted

comment:12 Changed 3 months ago by gh-mjungmath

  • Status changed from needs_review to needs_work

comment:13 Changed 3 months ago by git

  • Commit changed from 461ff656bc938720022b0bebdaef8ac1511916d0 to af5af8f7ed6cbc57832a64b75907f0fac054cb07

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

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

comment:14 Changed 3 months ago by git

  • Commit changed from af5af8f7ed6cbc57832a64b75907f0fac054cb07 to f0c6ee814ec3965b908073fd2dd47f2faead7e2b

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

f0c6ee8Trac #30191: minor fix

comment:15 Changed 3 months ago by git

  • Commit changed from f0c6ee814ec3965b908073fd2dd47f2faead7e2b to f1221da5168e9b3231e00bde01a138bb4f9dc5f0

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

f1221daTrac #30191: minor fix

comment:16 Changed 3 months ago by git

  • Commit changed from f1221da5168e9b3231e00bde01a138bb4f9dc5f0 to a042313372e11304b48195a3441cd0bdea0db0b7

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

f0c6ee8Trac #30191: minor fix
a042313Trac# 30191: wrong indentation reverted

comment:17 Changed 3 months ago by git

  • Commit changed from a042313372e11304b48195a3441cd0bdea0db0b7 to 4cc92e908ce58c863330aac9001ddccbd76437d5

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

4cc92e9Trac #30191: comp check unified to if comp

comment:18 Changed 3 months ago by gh-mjungmath

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

Last edited 3 months ago by gh-mjungmath (previous) (diff)

comment:19 Changed 3 months ago by gh-mjungmath

  • Status changed from needs_work to needs_review

comment:20 Changed 3 months ago by git

  • Commit changed from 4cc92e908ce58c863330aac9001ddccbd76437d5 to f98a47b9bc2302325a73ad1774dab26dc8a185d5

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

f98a47bTrac #30191: unused ScalarField import removed

comment:21 Changed 3 months ago by gh-mjungmath

Patchbot is green. Awaiting approval. :)

comment:22 Changed 3 months ago by tscrim

  • Reviewers set to Travis Scrimshaw

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.

comment:23 follow-up: Changed 3 months ago by gh-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.

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.

Last edited 3 months ago by gh-mjungmath (previous) (diff)

comment:24 in reply to: ↑ 23 ; follow-ups: Changed 3 months ago by tscrim

Replying to gh-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.

Last edited 3 months ago by tscrim (previous) (diff)

comment:25 in reply to: ↑ 24 Changed 3 months ago by gh-mjungmath

Replying to tscrim:

Replying to gh-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.

comment:26 Changed 3 months ago by git

  • Commit changed from f98a47b9bc2302325a73ad1774dab26dc8a185d5 to c34e4810fa3c8eea494ec67756987a7585e34a1e

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

c34e481Trac #30191: remove in ZZ check

comment:27 Changed 3 months ago by gh-mjungmath

Let's wait for the patchbot's response.

comment:28 Changed 3 months ago by tscrim

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

comment:29 follow-up: Changed 3 months ago by 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.

Last edited 3 months ago by egourgoulhon (previous) (diff)

comment:30 in reply to: ↑ 29 Changed 3 months ago by gh-mjungmath

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()
Last edited 3 months ago by gh-mjungmath (previous) (diff)

comment:31 follow-up: Changed 3 months ago by 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. (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.)

comment:32 in reply to: ↑ 31 ; follow-up: Changed 3 months ago by gh-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?

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

comment:33 in reply to: ↑ 32 ; follow-up: Changed 3 months ago by egourgoulhon

Replying to gh-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.

comment:34 in reply to: ↑ 33 Changed 3 months ago by gh-mjungmath

Replying to egourgoulhon:

Replying to gh-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?

comment:35 Changed 3 months ago by tscrim

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

comment:36 Changed 3 months ago by git

  • Commit changed from c34e4810fa3c8eea494ec67756987a7585e34a1e to acc407cc24226e8bee44bf30b39c24c7be86cb02

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

65dff3dTrac #30191: Merge branch 'develop' into conversion_attributeerror_fix
acc407cTrac #30191: try-except block for zero check

comment:37 Changed 3 months ago by git

  • Commit changed from acc407cc24226e8bee44bf30b39c24c7be86cb02 to 0d27b57ae5b5fefa64c56aadf29ee15806b84c77

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

0d27b57Trac #30191: unnecessary comment block removed

comment:38 Changed 3 months ago by git

  • Commit changed from 0d27b57ae5b5fefa64c56aadf29ee15806b84c77 to f94bcbe14781945c7d0e6a2643710599d1239e38

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

f94bcbeTrac #30191: if blocks rearranged

comment:39 Changed 3 months ago by gh-mjungmath

What about this?

comment:40 in reply to: ↑ 24 Changed 3 months ago by gh-mjungmath

Replying to tscrim:

Replying to gh-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

Apparently, this isn't consistently implemented. For example:

sage: GF(5).zero() in SR
False

The conversion fails, too:

sage: SR(GF(5).zero())
Traceback (most recent call last):
...
TypeError: positive characteristic not allowed in symbolic computations

Notice that our setup is closer to the symbolic ring than the ring of integers.

So, should I keep the things like we have discussed, or do we want to overthink the behavior again?

I, personally, have no strong opinion on that. However, I'd prefer a consistent behavior as much as possible.

Last edited 3 months ago by gh-mjungmath (previous) (diff)

comment:41 follow-up: Changed 3 months ago by tscrim

I think this is good. I might make one additional change in MixedFormAlgebra:

+        if comp is None:
+            return self.element_class(self, name=name, latex_name=latex_name)
         try:
             if comp.is_trivial_zero():
                 return self.zero()
             if (comp - 1).is_trivial_zero():
                 return self.one()
         except AttributeError:
             if comp == 0:
                 return self.zero()
             if comp == 1:
                 return self.one()
         res = self.element_class(self, name=name, latex_name=latex_name)
-        if comp is None:
-            return res

That way the comp being none gets the priority as I would guess that is the more likely possibility. However, you know what is the more likely inputs, and this is a bit closer to bikeshedding anyways.

comment:42 in reply to: ↑ 41 Changed 3 months ago by gh-mjungmath

Replying to tscrim:

I think this is good. I might make one additional change in MixedFormAlgebra:

+        if comp is None:
+            return self.element_class(self, name=name, latex_name=latex_name)
         try:
             if comp.is_trivial_zero():
                 return self.zero()
             if (comp - 1).is_trivial_zero():
                 return self.one()
         except AttributeError:
             if comp == 0:
                 return self.zero()
             if comp == 1:
                 return self.one()
         res = self.element_class(self, name=name, latex_name=latex_name)
-        if comp is None:
-            return res

That way the comp being none gets the priority as I would guess that is the more likely possibility. However, you know what is the more likely inputs, and this is a bit closer to bikeshedding anyways.

I don't think the None check is even necessary:

sage: M = Manifold(2, 'M')
sage: X.<x,y> = M.chart()
sage: M.mixed_form_algebra()()
Mixed differential form zero on the 2-dimensional differentiable manifold M

comment:43 follow-up: Changed 3 months ago by tscrim

It may not be necessary, but I think it gives a faster code path. There is another option: to make sure the user at least passes something as comp (i.e., remove the default being None).

comment:44 in reply to: ↑ 43 ; follow-up: Changed 3 months ago by gh-mjungmath

Replying to tscrim:

It may not be necessary, but I think it gives a faster code path. There is another option: to make sure the user at least passes something as comp (i.e., remove the default being None).

I think, the first input is already mandatory no matter what:

sage: M = Manifold(2, 'M')
sage: X.<x,y> = M.chart()
sage: M.mixed_form_algebra()(comp=None)
Traceback (most recent call last)
...
TypeError: _element_constructor_() got multiple values for argument 'comp'

I am not sure what happens here. Probably that is the coercion framework handling the process.

I would say we remove this check. I don't see in which cases it might give a shortcut.

Last edited 3 months ago by gh-mjungmath (previous) (diff)

comment:45 in reply to: ↑ 44 ; follow-up: Changed 3 months ago by tscrim

Replying to gh-mjungmath:

Replying to tscrim:

It may not be necessary, but I think it gives a faster code path. There is another option: to make sure the user at least passes something as comp (i.e., remove the default being None).

I think, the first input is already mandatory no matter what:

sage: M = Manifold(2, 'M')
sage: X.<x,y> = M.chart()
sage: M.mixed_form_algebra()(comp=None)
Traceback (most recent call last)
...
TypeError: _element_constructor_() got multiple values for argument 'comp'

I am not sure what happens here. Probably that is the coercion framework handling the process.

It actually is a slightly annoying issue that has appeared elsewhere in Sage. The problem is the initial __call__ of the mixed form algebra A requires at least one input x defaulting to 0. So what happens here is a call of A._element_constructor_(x, comp=None). Try A(None), A(), and ZZ().

There is some reason for this IIRC; I think it has to do with the coercion framework and needing to create a default element to test stuff with. However, things might have evolved enough where this is not a necessity anymore, but that definitely deserves a separate ticket.

I would say we remove this check. I don't see in which cases it might give a shortcut.

Then it should be removed.

comment:46 in reply to: ↑ 45 Changed 3 months ago by gh-mjungmath

Replying to tscrim:

Replying to gh-mjungmath:

Replying to tscrim:

It may not be necessary, but I think it gives a faster code path. There is another option: to make sure the user at least passes something as comp (i.e., remove the default being None).

I think, the first input is already mandatory no matter what:

sage: M = Manifold(2, 'M')
sage: X.<x,y> = M.chart()
sage: M.mixed_form_algebra()(comp=None)
Traceback (most recent call last)
...
TypeError: _element_constructor_() got multiple values for argument 'comp'

I am not sure what happens here. Probably that is the coercion framework handling the process.

It actually is a slightly annoying issue that has appeared elsewhere in Sage. The problem is the initial __call__ of the mixed form algebra A requires at least one input x defaulting to 0. So what happens here is a call of A._element_constructor_(x, comp=None). Try A(None), A(), and ZZ().

There is some reason for this IIRC; I think it has to do with the coercion framework and needing to create a default element to test stuff with. However, things might have evolved enough where this is not a necessity anymore, but that definitely deserves a separate ticket.

Thinking about it, this behavior is not unreasonable, is it? It ensures, also for the user, that the element he/she gets is already completely well-defined and ready for use. If one needs a "unprepared" element so badly, one can still use element_class, as it is performed e.g. in M.tensor_field.

Anyways, this should be discussed elsewhere, yes.

I would say we remove this check. I don't see in which cases it might give a shortcut.

Then it should be removed.

At least w.r.t. the behavior we have encountered.

Last edited 3 months ago by gh-mjungmath (previous) (diff)

comment:47 Changed 3 months ago by gh-mjungmath

The doctests fail because:

Traceback (most recent call last):
...
    if (comp - 1).is_trivial_zero():
TypeError: unsupported operand type(s) for -: 'list' and 'int'

Not surprising. Any suggestions?

comment:48 Changed 3 months ago by git

  • Commit changed from f94bcbe14781945c7d0e6a2643710599d1239e38 to 28dec692737c1ce7e9fadb753ff3b8b1c378d4c2

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

28dec69Trac #30191: doctest errors fixed

comment:49 Changed 3 months ago by gh-mjungmath

The last commit fixed all my doctest errors. Please check carefully. Do you agree proceeding this way in automorphismfield_group.py?


New commits:

28dec69Trac #30191: doctest errors fixed

comment:50 Changed 3 months ago by tscrim

  • Status changed from needs_review to positive_review

I think it is good.

Also, in response to comment:46, this is what an_element() is for.

comment:51 Changed 3 months ago by gh-mjungmath

Thanks for the review.

comment:52 Changed 3 months ago by gh-mjungmath

  • Reviewers changed from Travis Scrimshaw to Travis Scrimshaw, Eric Gourgoulhon

comment:53 Changed 3 months ago by gh-mjungmath

  • Status changed from positive_review to needs_work
Last edited 3 months ago by gh-mjungmath (previous) (diff)

comment:54 Changed 3 months ago by gh-mjungmath

  • Status changed from needs_work to positive_review
Last edited 3 months ago by gh-mjungmath (previous) (diff)

comment:55 Changed 3 months ago by vbraun

  • Branch changed from u/gh-mjungmath/conversion_attributeerror_fix to 28dec692737c1ce7e9fadb753ff3b8b1c378d4c2
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.