feat(NumpyModel): __eq__ and dump/load support for nested NumpyModels
#27
Conversation
caniko
added
enhancement
There was a problem hiding this comment.
Thank you for the PR, and welcome as one of the first contributors to the project! 🚀
This looks like a good first draft, please look at my comments.
Don't hesitate to mark your PR as a draft unless you are absolutely certain your code is ready for a final review before being merged.
There was a problem hiding this comment.
Same as the other test file:
- Move to correct file
- Use
unittest
| import numpy as np | ||
|
|
||
|
|
||
| class TestEQ: |
There was a problem hiding this comment.
Can we also test __hash__? Provided frozen=True; still not sure if numpy arrays can be hashed.
| for k, v in self.model_dump(exclude_unset=True).items(): | ||
| if isinstance(v, np.ndarray): | ||
| @staticmethod | ||
| def _dump_numpy_split_dict(model: "NumpyModel") -> tuple[dict[str, np.ndarray], dict[str, Any]]: |
There was a problem hiding this comment.
This function is complicated enough for it to warrant a doc string.
| if isinstance(v, np.ndarray): | ||
| @staticmethod | ||
| def _dump_numpy_split_dict(model: "NumpyModel") -> tuple[dict[str, np.ndarray], dict[str, Any]]: | ||
| ndarray_field_to_array = dict[str, np.ndarray]() |
There was a problem hiding this comment.
Don't parametrize the dict, but add type hints to the variable itself, that way we stay close to PEP8:
| ndarray_field_to_array = dict[str, np.ndarray]() | |
| ndarray_field_to_array: dict[str, np.ndarray] = {} |
Apply this to the others please
| for sub_k_other, sub_v_other in submodel_other_field_to_value.items(): | ||
| other_field_to_value[f"{k}/{sub_k_other}"] = sub_v_other | ||
| elif isinstance(v, np.ndarray): | ||
| ndarray_field_to_array[k] = v |
| from pathlib import Path | ||
|
|
||
|
|
||
| class TestDump: |
There was a problem hiding this comment.
Please inherit from unittest.IsolatedTestCase
| c.dump(path, "multilevel_model_test") | ||
|
|
||
| new_c = C.load(path, "multilevel_model_test") | ||
| assert new_c == c No newline at end of file |
| other_ndarray_field_to_array, other_other_field_to_value = other._dump_numpy_split_dict() | ||
|
|
||
| self_ndarray_field_to_array, self_other_field_to_value = self._dump_numpy_split_dict(self) | ||
| other_ndarray_field_to_array, other_other_field_to_value = other._dump_numpy_split_dict(self) |
There was a problem hiding this comment.
Whitespace on next line as before, please.
|
|
||
|
|
||
| def extract_dirs_to_dict(dir_dict: dict[str, Any]) -> dict[str, Any]: | ||
| """ Extract from a flat dict of {a/b/c : value} to {a: {b: {c: value}}}""" |
There was a problem hiding this comment.
You need to explain your nested dump and load strategy in the code somewhere so that newcomers can understand how different components work together to provide the respective utility to load and dump. Use doc strings, and not comments.
You also need to include a description for each argument.
caniko
changed the title
__eq__ and dump/load
caniko
changed the title
__eq__ and dump/load__eq__ and dump/load support for nested NumpyModels
|
@alonks1234 will you finalize your PR? |
|
Abandoned by author. |
2 participants
Adds support for equality comparison and dump/load of nested NumpyModels.
The implementation is a recursive conversion of dicts of fields containing other dicts to a single "flat" dict matching fields to paths, which correspond nicely with file paths.
Fixes:
#25, #26