Skip to content

Commit e3bd6ad

Browse files
gdementengdementen
committed
FIX: added warning for unsafe casting in CheckedSession (fixes larray-project#1168)
had to rework the interface between Session and CheckedSession
1 parent a350006 commit e3bd6ad

4 files changed

Lines changed: 221 additions & 113 deletions

File tree

‎doc/source/changes/version_0_35_1.rst.inc‎

Lines changed: 25 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -49,7 +49,31 @@ New features
4949
Miscellaneous improvements
5050
^^^^^^^^^^^^^^^^^^^^^^^^^^
5151

52-
* improved something.
52+
* added warning when doing unsafe implicit type conversions (for example from
53+
float to integers) when setting CheckedSession fields (closes :issue:`1168`).
54+
55+
>>> a = Axis('a=a0,a1')
56+
>>> class MySession(CheckedSession):
57+
... my_field: CheckedArray(a, dtype=int)
58+
>>> good_value = ndtest(a)
59+
>>> good_value
60+
a a0 a1
61+
0 1
62+
>>> s = MySession(my_field=good_value)
63+
>>> bad_value = good_value * 1.6
64+
>>> bad_value
65+
a a0 a1
66+
0.0 1.6
67+
>>> s.my_field = bad_value
68+
Array 'my_field' was declared with dtype int64 but got value with float64 dtype.
69+
70+
It will be converted to int64 like in previous larray versions but this is
71+
not a safe operation (some information could be lost in the conversion).
72+
73+
If you want to keep doing this conversion and silence this warning, please
74+
convert the value explicitly using value.astype(<declared_type>).
75+
76+
This warning will become an error in a future version of larray.
5377

5478

5579
Fixes

‎larray/core/checked.py‎

Lines changed: 85 additions & 41 deletions
Original file line numberDiff line numberDiff line change
@@ -61,6 +61,19 @@ def __init__(self, *args, **kwargs):
6161
# TypeAliasType, # from `typing_extensions`
6262
)
6363

64+
UNSAFE_CAST_WARNING_TEMPLATE = """
65+
Array '{name}' was declared with dtype {expected_dtype} but got value with \
66+
{value_dtype} dtype.
67+
68+
It will be converted to {expected_dtype} like in previous larray versions but \
69+
this is
70+
not a safe operation (some information could be lost in the conversion).
71+
72+
If you want to keep doing this conversion and silence this warning, please
73+
convert the value explicitly using value.astype(<declared_type>).
74+
75+
This warning will become an error in a future version of larray."""
76+
6477

6578
def CheckedArray(axes: AxisCollection, dtype: np.dtype = float) -> Type[Array]:
6679
"""
@@ -113,6 +126,17 @@ def validate_array(value: Any, info: ValidationInfo) -> Array:
113126
value = value.expand(expected_axes)
114127
# check dtype
115128
if value.dtype != dtype:
129+
if not np.can_cast(value.dtype, dtype, 'safe'):
130+
# TODO: in a later version, turn to an exception
131+
stacklevel = info.context.get("stacklevel", 2)
132+
warn_msg = UNSAFE_CAST_WARNING_TEMPLATE.format(
133+
name=name,
134+
expected_dtype=dtype,
135+
value_dtype=value.dtype
136+
)
137+
warnings.warn(warn_msg,
138+
category=FutureWarning,
139+
stacklevel=stacklevel)
116140
value = value.astype(dtype)
117141
return value
118142
else:
@@ -204,17 +228,20 @@ class CheckedSession(Session, BaseModel, metaclass=LArrayModelMetaclass):
204228
205229
>>> class ModelVariables(CheckedSession):
206230
... # --- declare variables with defined types ---
207-
... # Their values will be defined at runtime but must match the specified type.
231+
... # Their values will be defined at runtime but must match the
232+
... # specified type.
208233
... birth_rate: Array
209234
... births: Array
210235
... # --- declare variables with a default value ---
211-
... # The default value will be used to set the variable if no value is passed at instantiation (see below).
212-
... # Their type is deduced from their default value and cannot be changed at runtime.
236+
... # The default value will be used to set the variable if no value
237+
... # is passed at instantiation (see below).
238+
... # Their type is deduced from their default value and cannot be
239+
... # changed at runtime.
213240
... target_age = AGE[:2] >> '0-2'
214241
... population = zeros((AGE, GENDER, TIME), dtype=int)
215242
... # --- declare checked arrays ---
216-
... # The checked arrays have axes assumed to be "frozen", meaning they are
217-
... # constant all along the execution of the program.
243+
... # The checked arrays have axes assumed to be "frozen", meaning
244+
... # they are constant over the whole execution of the program.
218245
... mortality_rate: CheckedArray((AGE, GENDER))
219246
... # For checked arrays, the default value can be given as a scalar.
220247
... # Optionally, a dtype can be also specified (defaults to float).
@@ -287,9 +314,11 @@ class CheckedSession(Session, BaseModel, metaclass=LArrayModelMetaclass):
287314
memory used: 1.89 Kb
288315
>>> # now let's try to do the same for deaths.
289316
>>> m.mortality_rate = full((AGE, GENDER), fill_value=sequence(AGE, inc=0.02))
290-
>>> # here the result of the multiplication of the 'population' array by the 'mortality_rate' array
291-
>>> # is automatically converted to an integer array
292-
>>> m.deaths = m.population * m.mortality_rate
317+
>>> # the result of this multiplication is a float array
318+
>>> deaths = m.population * m.mortality_rate
319+
>>> # because m.deaths was declared as a CheckedArray with dtype=int,
320+
>>> # we need to convert it to an integer array to avoid a warning
321+
>>> m.deaths = (m.population * m.mortality_rate).astype(int)
293322
>>> print(m.deaths.info) # doctest: +SKIP
294323
11 x 2 x 11
295324
age [11]: 0 1 2 ... 8 9 10
@@ -332,35 +361,50 @@ def __init__(self, *args, meta=None, **kwargs):
332361
# is given as only argument
333362
# TODO: refactor Session.load() to use a private function which returns the handler directly
334363
# so that we can get the items out of it and avoid this
335-
input_data = dict(Session(*args, **kwargs))
364+
input_session = Session(*args, **kwargs)
365+
input_data = input_session._objects
366+
self._initialize_attributes(input_data, stacklevel=2)
367+
368+
@classmethod
369+
def _create_instance_from_dict(cls, data: dict, stacklevel=3) -> Session:
370+
try:
371+
inst = object.__new__(cls)
372+
Session.__init__(inst)
373+
inst._initialize_attributes(data, stacklevel=stacklevel)
374+
return inst
375+
except Exception as e:
376+
print(f"failed to create CheckedSession ({e}) => return normal Session")
377+
return Session(data)
378+
379+
def _initialize_attributes(self, data: dict, stacklevel: int):
380+
data = data.copy()
336381
# --- declared variables
337382
for name, field in self.__class__.model_fields.items():
338-
value = input_data.pop(name, NOT_LOADED)
339-
383+
value = data.pop(name, NOT_LOADED)
384+
skip_validation = False
340385
if value is NOT_LOADED:
341386
if field.default is PydanticUndefined:
342-
warnings.warn(f"No value passed for the declared variable '{name}'",
343-
stacklevel=2)
344387
# we actually use NOT_LOADED as the value
345-
self.__setattr__(name, value, skip_frozen=True, skip_validation=True)
388+
warnings.warn(f"No value passed for the declared variable '{name}'",
389+
stacklevel=stacklevel + 1)
390+
skip_validation = True
346391
else:
347-
self.__setattr__(name, field.default, skip_frozen=True)
392+
value = field.default
393+
394+
if skip_validation:
395+
super().__setattr__(name, value)
348396
else:
349-
self.__setattr__(name, value, skip_frozen=True)
397+
self.__setattr__(name, value, skip_frozen=True, stacklevel=stacklevel + 1)
350398

351399
# --- undeclared variables
352-
for name, value in input_data.items():
353-
self.__setattr__(name, value, skip_frozen=True, stacklevel=2)
400+
for name, value in data.items():
401+
self.__setattr__(name, value, skip_frozen=True, stacklevel=stacklevel + 1)
354402

355403
# code of the method below has been partly borrowed from pydantic.BaseModel.__setattr__()
356404
def _check_key_value(self, name: str,
357405
value: Any,
358406
skip_frozen: bool,
359-
skip_validation: bool,
360407
stacklevel: int) -> Any:
361-
if skip_validation:
362-
return value
363-
364408
cls = self.__class__
365409
cls_name = cls.__name__
366410
model_config = cls.model_config
@@ -382,6 +426,7 @@ def _check_key_value(self, name: str,
382426
field_info = model_fields[name]
383427
field_type = field_info.annotation
384428
if field_type is None:
429+
assert False, f"None field_type for {name} ({value})"
385430
return value
386431

387432
# Annotated[T, x] => field_info.metadata == (x,)
@@ -399,9 +444,11 @@ def _check_key_value(self, name: str,
399444

400445
# see https://docs.pydantic.dev/2.12/concepts/types/#custom-types
401446
# for more details about TypeAdapter
402-
adapter = TypeAdapter(field_type, config=self.model_config)
447+
adapter = TypeAdapter(field_type, config=model_config)
403448
try:
404-
value = adapter.validate_python(value, context={'name': name})
449+
# pydantic machinery adds a few stack frames
450+
validate_context = {'name': name, 'stacklevel': stacklevel + 3}
451+
value = adapter.validate_python(value, context=validate_context)
405452
except ValidationError as e:
406453
error = e.errors()[0]
407454
msg = f"Error while assigning value to variable '{name}':\n"
@@ -418,32 +465,29 @@ def _check_key_value(self, name: str,
418465

419466
def _update_from_iterable(self, it):
420467
for k, v in it:
421-
self.__setitem__(k, v, stacklevel=3)
468+
self.__setattr__(k, v, stacklevel=3)
422469

423-
def __setitem__(self, key, value, skip_frozen=False, skip_validation=False, stacklevel=1):
470+
def __setitem__(self, key, value):
424471
if key == 'meta':
425472
raise ValueError(
426473
"Sessions cannot contain any object named 'meta'. "
427474
"To modify the session metadata, use "
428475
"'session.meta = value' instead.")
429-
value = self._check_key_value(key, value, skip_frozen, skip_validation, stacklevel=stacklevel + 1)
430-
# we need to keep the attribute in sync
431-
# TODO: I don't think this is specific to CheckedSession, so either
432-
# we should do it in Session too or not do it here.
433-
object.__setattr__(self, key, value)
434-
self._objects[key] = value
435-
436-
def __setattr__(self, key, value, skip_frozen=False, skip_validation=False, stacklevel=1):
476+
value = self._check_key_value(key, value,
477+
skip_frozen=False,
478+
stacklevel=2)
479+
super().__setitem__(key, value)
480+
# object.__setattr__(self, key, value)
481+
482+
def __setattr__(self, key, value, skip_frozen=False, stacklevel=1):
437483
if key == 'meta':
438484
object.__setattr__(self, key, value)
439485
return
440-
441-
value = self._check_key_value(key, value, skip_frozen, skip_validation, stacklevel=stacklevel + 1)
442-
# we need to keep the attribute in sync
443-
# TODO: I don't think this is specific to CheckedSession, so either
444-
# we should do it in Session too or not do it here.
445-
object.__setattr__(self, key, value)
446-
self._objects[key] = value
486+
value = self._check_key_value(key, value,
487+
skip_frozen,
488+
stacklevel=stacklevel + 1)
489+
super().__setattr__(key, value)
490+
# object.__setattr__(self, key, value)
447491

448492
def __getstate__(self) -> Dict[str, Any]:
449493
return {'__dict__': self.__dict__}

‎larray/core/session.py‎

Lines changed: 59 additions & 54 deletions
Original file line numberDiff line numberDiff line change
@@ -310,8 +310,9 @@ def __delitem__(self, key) -> None:
310310
del self._objects[key]
311311

312312
def __getattr__(self, key) -> Any:
313-
if key in self._objects:
314-
return self._objects[key]
313+
data = object.__getattribute__(self, '_objects')
314+
if key in data:
315+
return data[key]
315316
else:
316317
raise AttributeError(f"'{self.__class__.__name__}' object has no attribute '{key}'")
317318

@@ -1013,44 +1014,48 @@ def __repr__(self) -> str:
10131014
def __len__(self) -> int:
10141015
return len(self._objects)
10151016

1017+
@classmethod
1018+
def _create_instance_from_dict(cls, data: dict, stacklevel=3) -> 'Session':
1019+
return cls(data)
1020+
10161021
# binary operations are dispatched element-wise to all arrays (we consider Session as an array-like)
1017-
def _binop(opname, arrays_only=True):
1022+
def _compute_binop(self, opname, other, cls_filter) -> dict:
10181023
opfullname = f'__{opname}__'
1019-
1020-
def opmethod(self, other) -> 'Session':
1021-
self_keys = set(self.keys())
1022-
all_keys = list(self.keys())
1023-
if not isinstance(other, Array) and hasattr(other, 'keys'):
1024-
all_keys += [n for n in other.keys() if n not in self_keys]
1025-
with np.errstate(call=_session_float_error_handler):
1026-
res = []
1027-
for name in all_keys:
1028-
self_item = self.get(name, nan)
1029-
other_operand = other.get(name, nan) if hasattr(other, 'get') else other
1030-
if arrays_only and not isinstance(self_item, Array):
1031-
res_item = self_item
1032-
else:
1024+
inv_opname = f'__{inverseop(opname)}__'
1025+
all_keys = list(self.keys())
1026+
if not isinstance(other, Array) and hasattr(other, 'keys'):
1027+
# we cannot simply use all_keys += self.keys() - other.keys()
1028+
# because we need to keep ordering
1029+
self_keys_set = set(all_keys)
1030+
all_keys.extend([k for k in other.keys() if k not in self_keys_set])
1031+
1032+
res = {}
1033+
with np.errstate(call=_session_float_error_handler):
1034+
for key in all_keys:
1035+
self_item = self.get(key, nan)
1036+
other_item = other.get(key, nan) if hasattr(other, 'get') else other
1037+
if cls_filter is not None and not isinstance(self_item, cls_filter):
1038+
res_item = self_item
1039+
else:
1040+
try:
1041+
res_item = getattr(self_item, opfullname)(other_item)
1042+
# TypeError for str arrays, ValueError for incompatible axes, ...
1043+
except Exception:
1044+
res_item = nan
1045+
if res_item is NotImplemented:
10331046
try:
1034-
res_item = getattr(self_item, opfullname)(other_operand)
1047+
res_item = getattr(other_item, inv_opname)(self_item)
10351048
# TypeError for str arrays, ValueError for incompatible axes, ...
10361049
except Exception:
10371050
res_item = nan
1038-
# this should only ever happen when self_array is a non Array (eg. nan)
1039-
if res_item is NotImplemented:
1040-
inv_opname = f'__{inverseop(opname)}__'
1041-
try:
1042-
res_item = getattr(other_operand, inv_opname)(self_item)
1043-
# TypeError for str arrays, ValueError for incompatible axes, ...
1044-
except Exception:
1045-
res_item = nan
1046-
res.append((name, res_item))
1047-
try:
1048-
# XXX: print a warning?
1049-
ses = self.__class__(res)
1050-
except Exception:
1051-
ses = Session(res)
1052-
return ses
1053-
opmethod.__name__ = opfullname
1051+
res[key] = res_item
1052+
return res
1053+
1054+
def _binop(opname, default_cls_filter=Array):
1055+
def opmethod(self, other, cls_filter=default_cls_filter) -> 'Session':
1056+
dict_res = self._compute_binop(opname, other, cls_filter)
1057+
return self._create_instance_from_dict(dict_res)
1058+
opmethod.__name__ = f'__{opname}__'
10541059
return opmethod
10551060

10561061
__add__ = _binop('add')
@@ -1062,30 +1067,30 @@ def opmethod(self, other) -> 'Session':
10621067
__truediv__ = _binop('truediv')
10631068
__rtruediv__ = _binop('rtruediv')
10641069

1065-
__eq__ = _binop('eq', arrays_only=False)
1066-
__ne__ = _binop('ne', arrays_only=False)
1070+
__eq__ = _binop('eq', default_cls_filter=None)
1071+
__ne__ = _binop('ne', default_cls_filter=None)
10671072

10681073
# element-wise method factory
10691074
# unary operations are (also) dispatched element-wise to all arrays
1070-
def _unaryop(opname):
1075+
def _compute_unaryop(self, opname, cls_filter):
10711076
opfullname = f'__{opname}__'
1077+
res = {}
1078+
with np.errstate(call=_session_float_error_handler):
1079+
for k, v in self.items():
1080+
try:
1081+
res_item = getattr(v, opfullname)()
1082+
except Exception:
1083+
res_item = nan
1084+
res[k] = res_item
1085+
return res
10721086

1073-
def opmethod(self) -> 'Session':
1074-
with np.errstate(call=_session_float_error_handler):
1075-
res = []
1076-
for k, v in self.items():
1077-
try:
1078-
res_array = getattr(v, opfullname)()
1079-
except Exception:
1080-
res_array = nan
1081-
res.append((k, res_array))
1082-
try:
1083-
# XXX: print a warning?
1084-
ses = self.__class__(res)
1085-
except Exception:
1086-
ses = Session(res)
1087-
return ses
1088-
opmethod.__name__ = opfullname
1087+
def _unaryop(opname):
1088+
# TODO: change default cls_filter to Array (using None for now
1089+
# to avoid changing behavior in a bugfix release)
1090+
def opmethod(self, cls_filter=None) -> 'Session':
1091+
dict_res = self._compute_unaryop(opname, cls_filter)
1092+
return self._create_instance_from_dict(dict_res)
1093+
opmethod.__name__ = f'__{opname}__'
10891094
return opmethod
10901095

10911096
__neg__ = _unaryop('neg')
@@ -1763,4 +1768,4 @@ def arrays(depth=0, include_private=False, meta=None) -> Session:
17631768
return Session({k: v for k, v in combined_vars if isinstance(v, Array)}, meta=meta)
17641769

17651770

1766-
_session_float_error_handler = float_error_handler_factory(4)
1771+
_session_float_error_handler = float_error_handler_factory(5)

0 commit comments

Comments
 (0)