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.

Sign up for GitHub

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

Consistent validation with complex number to float type coercion #1574

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Consistent validation with complex number to float type coercion #1574

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
675a214
Consistent validation with complex number to float type coercion
ambroseling Dec 8, 2024
8e5b5ff
Improved patch coverage
ambroseling Dec 8, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions src/input/input_python.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -291,6 +291,15 @@ impl<'py> Input<'py> for Bound<'py, PyAny> {
return Ok(ValidationMatch::exact(EitherFloat::Py(float.clone())));
}

if self.is_instance_of::<PyComplex>() {
if strict {
return Err(ValError::new(ErrorTypeDefaults::FloatType, self));
}
let real = self.getattr(intern!(self.py(), "real")).unwrap();
Copy link
Contributor

@changhc changhc Dec 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Perhaps we should raise a warning here. In numpy's case, they do raise a custom ComplexWarning for discarding the imaginary part. At least we should document whatever coercion is implemented here because there can be a few possibilities for complex -> float:

  • no coercion is allowed: consistent with the default python behaviour, e.g. float(complex(1, 2)) gives an error, even when the imaginary part is 0
  • coercion is allowed only when the imaginary part is 0: this is what I would actually expect when coercions can actually take place
  • coercion is allowed for all complex numbers, done by simply dropping the imaginary part: this seems to be a numpy-only behaviour and I'm not sure if non-numpy users would expect this

...if we really want to implement this coercion. Please take a look at my comment. pydantic/pydantic#10430 (comment)

Copy link
Contributor

Choose a reason for hiding this comment

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

I am somewhat open to lax mode allowing complex -> float if the imaginary component is zero, similarly we allow datetime -> date if the datetime is at midnight.

I think we should never drop a nonzero imaginary part.

let float = real.extract::<f64>().unwrap();
return Ok(ValidationMatch::lax(EitherFloat::F64(float)));
}

if !strict {
if let Some(s) = maybe_as_string(self, ErrorTypeDefaults::FloatParsing)? {
// checking for bytes and string is fast, so do this before isinstance(float)
Expand Down
18 changes: 16 additions & 2 deletions tests/validators/test_float.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,10 @@
(False, 0),
('wrong', Err('Input should be a valid number, unable to parse string as a number [type=float_parsing')),
([1, 2], Err('Input should be a valid number [type=float_type, input_value=[1, 2], input_type=list]')),
(1 + 0j, 1.0),
(2.5 + 0j, 2.5),
(3 + 1j, 3.0),
(1j, 0),
],
)
def test_float(py_and_json: PyAndJson, input_value, expected):
Expand All @@ -37,7 +41,10 @@ def test_float(py_and_json: PyAndJson, input_value, expected):
with pytest.raises(ValidationError, match=re.escape(expected.message)):
v.validate_test(input_value)
else:
output = v.validate_test(input_value)
if isinstance(input_value, complex):
output = v.validate_python(input_value)
else:
output = v.validate_test(input_value)
assert output == expected
assert isinstance(output, float)

Expand All @@ -52,14 +59,21 @@ def test_float(py_and_json: PyAndJson, input_value, expected):
(42.5, 42.5),
('42', Err("Input should be a valid number [type=float_type, input_value='42', input_type=str]")),
(True, Err('Input should be a valid number [type=float_type, input_value=True, input_type=bool]')),
(1 + 0j, Err('Input should be a valid number [type=float_type, input_value=(1+0j), input_type=complex]')),
(2.5 + 0j, Err('Input should be a valid number [type=float_type, input_value=(2.5+0j), input_type=complex]')),
(3 + 1j, Err('Input should be a valid number [type=float_type, input_value=(3+1j), input_type=complex]')),
(1j, Err('Input should be a valid number [type=float_type, input_value=1j, input_type=complex]')),
],
ids=repr,
)
def test_float_strict(py_and_json: PyAndJson, input_value, expected):
v = py_and_json({'type': 'float', 'strict': True})
if isinstance(expected, Err):
with pytest.raises(ValidationError, match=re.escape(expected.message)):
v.validate_test(input_value)
if isinstance(input_value, complex):
v.validate_python(input_value)
else:
v.validate_test(input_value)
else:
output = v.validate_test(input_value)
assert output == expected
Expand Down
Loading