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

Make Complex trait type validation duck-typed. #1594

Merged
merged 4 commits into from
Nov 15, 2021

Conversation

mdickinson
Copy link
Member

@mdickinson mdickinson commented Nov 8, 2021

This PR changes the validation of the Complex trait type to use a duck-typed approach instead of the current white-list-of-accepted-types approach. All values that were previously accepted in a Complex trait will continue to be accepted, but objects whose type implements __complex__ will now also be accepted. Essentially, Complex will accept any non-string-type that the complex built-in accepts.

This parallels similar changes made a while ago for the Int and Float trait types (see #104, #393), both of which already do duck-typed checks. It makes it clearer that the behaviour of the Complex trait type does not depend on whether NumPy is available or not, and it ensures that Complex will accept complex-like objects from other frameworks.

Implementation notes:

  • I've added a new "fast_validate" code, code 23, for validating complex numbers. Unfortunately we already have a "complex" element of the ValidateTraits enumeration, but the "complex" there refers to compound trait types (e.g., created using Either) rather than complex numbers. So the new enumeration value is called complex_number instead of complex.
  • For the BaseComplex validation, it's rather hard to write pure Python code that matches the behaviour we want (essentially, we want to accept exactly the set of things that the std. lib. cmath module functions accept - that's roughly the same as things that the complex constructor accepts, but excluding strings). Instead, I've added a C implementation of that logic, and a private function _validate_complex_number in traits.ctraits that exposes the logic to Python.

Checklist

  • Tests
  • Update API reference (docs/source/traits_api_reference)
  • Update User manual (docs/source/traits_user_manual)
    - [ ] Update type annotation hints in traits-stubs N/A

@mdickinson
Copy link
Member Author

mdickinson commented Nov 8, 2021

Note that BaseComplex does not currently accept NumPy complex types other that complex128 (or indeed float types other than float64); with this PR, it will. Here's the current behaviour on main (i.e., before this PR):

(traits) mdickinson@mirzakhani traits % python
Python 3.10.0 (default, Nov  7 2021, 21:08:03) [Clang 12.0.5 (clang-1205.0.22.11)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> from traits.api import BaseComplex, Complex, HasTraits
>>> class HasComplexTraits(HasTraits):
...     bc = BaseComplex()
...     c = Complex()
... 
>>> obj = HasComplexTraits()
>>> import numpy as np
>>> obj.bc = np.float32(1.0)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/Users/mdickinson/Enthought/ETS/traits/traits/trait_types.py", line 396, in validate
    self.error(object, name, value)
  File "/Users/mdickinson/Enthought/ETS/traits/traits/base_trait_handler.py", line 74, in error
    raise TraitError(
traits.trait_errors.TraitError: The 'bc' trait of a HasComplexTraits instance must be a complex number, but a value of 1.0 <class 'numpy.float32'> was specified.
>>> obj.c = np.float32(1.0)
>>> obj.bc = np.complex128(0)
>>> obj.bc = np.complex64(0)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/Users/mdickinson/Enthought/ETS/traits/traits/trait_types.py", line 396, in validate
    self.error(object, name, value)
  File "/Users/mdickinson/Enthought/ETS/traits/traits/base_trait_handler.py", line 74, in error
    raise TraitError(
traits.trait_errors.TraitError: The 'bc' trait of a HasComplexTraits instance must be a complex number, but a value of 0j <class 'numpy.complex64'> was specified.
>>> obj.c = np.complex64(0)

@@ -409,7 +407,7 @@ class Complex(BaseComplex):
"""

#: The C-level fast validator to use:
fast_validate = complex_fast_validate
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks like complex_fast_validate isnt used anywhere now. int_fast_validate and float_fast_validate aren't used anywhere either. are we keeping it around mainly because it is part of the public API?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are we keeping it around mainly because it is part of the public API

No, I'm just keeping them around to keep this PR simple and focused. I do indeed plan a follow-up PR that removes all three of int_fast_validate, float_fast_validate and complex_fast_validate. They're not part of the public API - they're not exposed in any api module. That alone should mean that we don't need to justify removing them, but I did also check all projects available to me to confirm that nothing I can see is using any of these.

I confess I was hoping to go further and remove the use of numpy in trait_types, but the Bool trait messes that up.

traits/tests/test_complex.py Show resolved Hide resolved
@rahulporuri
Copy link
Contributor

Note that BaseComplex does not currently accept NumPy complex types other that complex128 (or indeed float types other than float64); with this PR, it will.

IIUC, this is because BaseComplex.validate wasnt handling these cases whereas Complex used complex_fast_validate to handle these cases. Is that correct?

@mdickinson
Copy link
Member Author

IIUC, this is because BaseComplex.validate wasnt handling these cases whereas Complex used complex_fast_validate to handle these cases. Is that correct?

Yes.

@mdickinson mdickinson merged commit f6e00f2 into main Nov 15, 2021
@mdickinson mdickinson deleted the feature/duck-typed-complex-trait branch November 15, 2021 10:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants