-
Notifications
You must be signed in to change notification settings - Fork 247
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
add default comparison #1114
base: main
Are you sure you want to change the base?
add default comparison #1114
Conversation
fn compare_with_default(&self, py: Python, value: &PyAny) -> PyResult<bool> { | ||
if let Some(default) = self.get_default(py)? { | ||
if let Some(default_comparison) = &self.default_comparison { | ||
return default_comparison.call(py, (value, default), None)?.extract::<bool>(py); |
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'm passing the values as positional here, not sure if is the best way to call the comparison operator
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #1114 +/- ##
==========================================
+ Coverage 89.70% 89.72% +0.01%
==========================================
Files 106 106
Lines 16364 16380 +16
Branches 35 35
==========================================
+ Hits 14680 14697 +17
+ Misses 1677 1676 -1
Partials 7 7
... and 3 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
CodSpeed Performance ReportMerging #1114 will degrade performances by 13.46%Comparing Summary
Benchmarks breakdown
|
2a07995
to
d1f2343
Compare
please review |
Hi @andresliszt thanks for the PR! I spoke to @adriangb a bit this morning and we think this is potentially reasonable to move forward with; we can't see a better solution for the Also, on the linked issue I commented about |
Hi @davidhewitt ! I was thinking to expose it using pydantic import numpy as np
from pydantic import BaseModel, ConfigDict, Field
class Model(BaseModel):
my_field: np.ndarray = Field(default = np.array(), default_comparison = np.array_equal)
model_config = ConfigDict(arbitrary_types_allowed = True) |
That would definitely be the obvious choice. Some other idea could be to make a special import numpy as np
from pydantic import BaseModel, ConfigDict, Field, Default
class Model(BaseModel):
my_field: np.ndarray = Field(default = Default(np.array(), compare_as = np.array_equal))
model_config = ConfigDict(arbitrary_types_allowed = True) (credit @adriangb) |
After I sent my PR i started to think that excluding defaults is not the only place where the python Model(my_field = np.array([1,2,3])) == Model(my_field = np.array([1,2,3]))
>>> ValueError: The truth value of an array with more than one element is ambiguous. Use a.any() or a.all() I think adding |
Interesting observation, I like the generality of this solution. https://www.attrs.org/en/stable/comparison.html#customization |
I was poking around My first idea was to create a class, for instance class FieldComparator:
def __init__(self, value: Any, compare_as: Callable):
self.value = value
self.comapre_as = compare_as
def __eq__(self, other):
return self.compare_as(self.value, other) And replace all values in # Naive way
self_dict = {k: FieldComparator(value = v, compare_as = self.model_fields[k].compare_as) if self.model_fields[k].compare_as is not None else v for k,v in self.__dict__.items()} Then using I didn't try this and i don't know how it impacts in the performance, but what about creating a new attribute in the Rust side, say |
This PR tries to solve this pydantic issue . In my work I use objects of the type
np.ndarray
orpd.DataFrame
a lot and excluding the defaults values has been a bit annoying due to the error mentioned in the issue. The idea of this PR is to add an extra optional argument inField
that I calleddefault_comparison
, which is a callable that receives two arguments value and default and replaces the__eq__
call of the object with a call to this functionChecklist
pydantic-core
(except for expected changes)Selected Reviewer: @adriangb