Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

'' in CC fails #35607

Closed
2 tasks done
GMS103 opened this issue May 3, 2023 · 6 comments · Fixed by #35610
Closed
2 tasks done

'' in CC fails #35607

GMS103 opened this issue May 3, 2023 · 6 comments · Fixed by #35610
Labels
Milestone

Comments

@GMS103
Copy link
Member

GMS103 commented May 3, 2023

Is there an existing issue for this?

  • I have searched the existing issues for a bug report that matches the one I want to file, without success.

Did you read the documentation and troubleshoot guide?

  • I have read the documentation and troubleshoot guide

Environment

- OS:  macOS (not relevant I think)
- Sage Version:  9.8

Steps To Reproduce

sage: '' in CC

Expected Behavior

False

Actual Behavior

Traceback (most recent call last):

  File /private/var/tmp/sage-9.8-current/local/var/lib/sage/venv-python3.11.1/lib/python3.11/site-packages/IPython/core/interactiveshell.py:3433 in run_code
    exec(code_obj, self.user_global_ns, self.user_ns)

  Cell In [2], line 1
    '' in CC

  File sage/structure/parent.pyx:1161 in sage.structure.parent.Parent.__contains__ (build/cythonized/sage/structure/parent.c:10544)
    x2 = self(x)

  File sage/rings/complex_mpfr.pyx:484 in sage.rings.complex_mpfr.ComplexField_class.__call__ (build/cythonized/sage/rings/complex_mpfr.c:7418)
    return Parent.__call__(self, x)

  File sage/structure/parent.pyx:896 in sage.structure.parent.Parent.__call__ (build/cythonized/sage/structure/parent.c:9516)
    return mor._call_(x)

  File sage/structure/coerce_maps.pyx:161 in sage.structure.coerce_maps.DefaultConvertMap_unique._call_ (build/cythonized/sage/structure/coerce_maps.c:4816)
    raise

  File sage/structure/coerce_maps.pyx:156 in sage.structure.coerce_maps.DefaultConvertMap_unique._call_ (build/cythonized/sage/structure/coerce_maps.c:4708)
    return C._element_constructor(x)

  File sage/rings/complex_mpfr.pyx:526 in sage.rings.complex_mpfr.ComplexField_class._element_constructor_ (build/cythonized/sage/rings/complex_mpfr.c:8038)
    sage_eval(x, locals={"I": self.gen()}))

  File /private/var/tmp/sage-9.8-current/local/var/lib/sage/venv-python3.11.1/lib/python3.11/site-packages/sage/misc/sage_eval.py:198 in sage_eval
    return eval(source, sage.all.__dict__, locals)

  File <string>
    
    ^
SyntaxError: invalid syntax

Additional Information

These work: '' in ZZ, '' in QQ, '' in RR (return False as expected).
See https://groups.google.com/g/sage-support/c/z1mB4LNhgHo/m/knNw4NjXAAAJ
Quoting Nils:

It is a little strange that an empty string is getting parsed to test for membership in CC.

@GMS103 GMS103 added the t: bug label May 3, 2023
@nbruin
Copy link
Contributor

nbruin commented May 4, 2023

This error does seem to be rather specific for CC. For instance, for '' in RR just returns False. The problem arises in CC.__contains__ which is just the generic routine. It tries CC('') (which indeed causes the Syntax error). This conversion is tried for all parents and is guarded by a try/except that catches (TypeError, ValueError, ArithmeticError), so the SyntaxError slips through.

The evaluation arises in CC._element_constructor:

            elif isinstance(x, str):
...
                # This should rather use a proper parser to validate input.
                # TODO: this is probably not the best and most
                # efficient way to do this.
                return ComplexNumber(self,
                                     sage_eval(x, locals={"I": self.gen()}))

This is guarded with some checking of the string. We can add testing for empty strings to that, but that won't solve the problem entirely:

sage: '1e2e3' in CC
SyntaxError: invalid decimal literal

I'd be a little apprehensive about adding SyntaxError to the list of caught errors in the super-generic __contains__.

@GMS103
Copy link
Member Author

GMS103 commented May 4, 2023

Sorry, I do not understand.
For any string s, should not s in CC return False "immediately"?
Why try CC(s)?

@nbruin
Copy link
Contributor

nbruin commented May 4, 2023

Yes, it could do that, but by that logic 1 in QQ would return False as well, because 1 lies in ZZ. It just happens to be coercible into QQ as well. I think that's the logic behind the generic routine also trying if the object can be successfully converted into the parent. It's definitely in line with other parents that `CC("1+I") can be used to create a complex number. I agree it's questionable whether that should be tried as part of a membership test, but it's a consequence of the architecture at the moment.

@GMS103
Copy link
Member Author

GMS103 commented May 4, 2023

I see.
Now I wonder about the consistency of this…

sage: s='1+I'
sage: CC(s)
1.00000000000000 + 1.00000000000000*I
sage: s in CC
False

@nbruin
Copy link
Contributor

nbruin commented May 5, 2023

Oh boy. That's an interesting one. Not really a consistency problem by the looks of it: strings not being complex numbers is fine. But why then raise an error if the string is not syntactical? It's probably testing if the original object is considered equal to its conversion into the parent (if that conversion exists).

sage: s="1+I"
sage: CC(s) == s
False

(indeed

shows that's exactly what happens. It would require devious trickery to avoid this from happening for CC, though. I don't think we want to special-case the containment method for CC)

vbraun pushed a commit that referenced this issue May 28, 2023
…t_constructor

    
<!-- Please provide a concise, informative and self-explanatory title.
-->
<!-- Don't put issue numbers in the title. Put it in the Description
below. -->
<!-- For example, instead of "Fixes #12345", use "Add a new method to
multiply two integers" -->

### 📚 Description

<!-- Describe your changes here in detail. -->
<!-- Why is this change required? What problem does it solve? -->
<!-- If this PR resolves an open issue, please link to it here. For
example "Fixes #12345". -->
Fixes #35607
<!-- If your change requires a documentation PR, please link it
appropriately. -->

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. It should be `[x]` not `[x
]`. -->

- [x] The title is concise, informative, and self-explanatory.
- [x] The description explains in detail what this PR is about.
- [x] I have linked a relevant issue or discussion.
- [ ] I have created tests covering the changes.
- [ ] I have updated the documentation accordingly.

### ⌛ Dependencies

<!-- List all open PRs that this PR logically depends on
- #12345: short description why this is a dependency
- #34567: ...
-->

<!-- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->
    
URL: #35610
Reported by: nbruin
Reviewer(s): Matthias Köppe
@mkoeppe mkoeppe added this to the sage-10.1 milestone May 28, 2023
@GMS103
Copy link
Member Author

GMS103 commented May 28, 2023

Thanks to everybody.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants