- Notifications
You must be signed in to change notification settings - Fork293
Consistent validation with complex number to float type coercion#1574
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:main
Are you sure you want to change the base?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
codecovbot commentedDec 8, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@## main #1574 +/- ##==========================================- Coverage 90.21% 89.63% -0.59%========================================== Files 106 112 +6 Lines 16339 17898 +1559 Branches 36 40 +4 ==========================================+ Hits 14740 16042 +1302- Misses 1592 1836 +244- Partials 7 20 +13
... and55 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
codspeed-hqbot commentedDec 8, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
CodSpeed Performance ReportMerging#1574 willnot alter performanceComparing Summary
|
The CI is failing due to a pyright version mismatch (trying to install v1.1.389). I've verified that this version exists in the npm registry but for some reason CI was unable to fetch it. This seems to be an infrastructure issue rather than a problem with my changes. Would it be possible to get some guidance on this or have the CI re-run? And it would be great if my changes can be reviewed. Thank you for your time! |
| if strict { | ||
| return Err(ValError::new(ErrorTypeDefaults::FloatType, self)); | ||
| } | ||
| let real = self.getattr(intern!(self.py(), "real")).unwrap(); |
There was a problem hiding this comment.
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 customComplexWarning 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)
There was a problem hiding this comment.
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 allowingcomplex ->floatif the imaginary component is zero, similarly we allowdatetime ->dateif the datetime is at midnight.
I think we should never drop a nonzero imaginary part.
Uh oh!
There was an error while loading.Please reload this page.
Change Summary
When validating floats in non-strict mode, this PR adds support for properly handling complex to float coercion by
by extracting their real component, this behavior is prohibited in strict mode and returns ValidationError. Appropriate unit tests are added to
test_floatandtest_float_strict.Related issue number
Fixespydantic/pydantic#10430
Checklist
pydantic-core(except for expected changes)Selected Reviewer:@davidhewitt