feat: add kwargs to creation functions to override properties #3746
Conversation
d3192d8 to
86d0511
Compare
src/scipp/core/like.py
Outdated
| dtype=obj.dtype, | ||
| with_variances=obj.variances is not None, | ||
| ) | ||
| new_values = zeros(**(default | kwargs)) |
There was a problem hiding this comment.
There is a subtlety with dims and shape. When you create zeros etc.., you can either use dims/shape or you can use sizes which is a dict mapping dim to shape.
Your implementation here would cause a clash of arguments if you do zeros_like(a, sizes={'x': 10, 'y': 5})
src/scipp/core/like.py
Outdated
| Create an object with uninitialized elements. | ||
| """ | ||
| new_values = ones( | ||
| default = dict( # noqa: C408 |
There was a problem hiding this comment.
Is the noqa because the linter is asking you to not use the dict() constructor but the {dims: obj.dims,...} syntax instead?
If so, why not keep the linter happy instead?
There was a problem hiding this comment.
Because I think it's clearer like this, it makes the diff smaller, but if you prefer I can change it to be a dict literal 🤷
b7a20e1 to
0c39ffa
Compare
src/scipp/core/like.py
Outdated
| default = { | ||
| 'unit': obj.unit, | ||
| 'dtype': obj.dtype, | ||
| 'with_variances': obj.variances is not None, | ||
| } | ||
| if 'sizes' not in kwargs: | ||
| default.setdefault('dims', obj.dims) | ||
| default.setdefault('shape', obj.shape) |
There was a problem hiding this comment.
Is the code here now common to all functions?
If so, how about moving all this to small helper and calling that inside each function?
tests/variable_creation_test.py
Outdated
|
|
||
| @pytest.mark.parametrize(('dims', 'shape'), [(['x'], (1,)), (['x', 'y'], (2, 3))]) | ||
| def test_full_like_with_dims_shape(dims, shape): | ||
| to_copy = sc.zeros(dims=["x", "y"], shape=(2, 2)) |
There was a problem hiding this comment.
Can you add a unit and/or dtype here, and check that this is propagated to the output of full_like?
Cause I think in the full_like below, we are sort of just making an entirely new array -- i.e. it is not obvious that any properties from to_copy are propagated to the result?
nvaytet
left a comment
There was a problem hiding this comment.
Looks good, just a final suggestion to make the docstrings link to the related functions.
| if 'sizes' not in kwargs: | ||
| default.setdefault('dims', obj.dims) | ||
| default.setdefault('shape', obj.shape) |
There was a problem hiding this comment.
What if someone passes, e.g., sizes and dims? Shouldn't we raise?
There was a problem hiding this comment.
If someone passes sizes and dims the scipp.ones method will raise because it received both sizes and dims, right?
There was a problem hiding this comment.
Yes, I guess one could rely on that.
|
While the approach of using |
Makes sense. I'll change it to use explicit argument names instead |
Co-authored-by: Neil Vaytet <39047984+nvaytet@users.noreply.github.com>
Co-authored-by: Neil Vaytet <39047984+nvaytet@users.noreply.github.com>
Co-authored-by: Neil Vaytet <39047984+nvaytet@users.noreply.github.com>
Co-authored-by: Neil Vaytet <39047984+nvaytet@users.noreply.github.com>
| sizes: dict[str, int] | None | _NoArgProvided = _no_arg_provided, | ||
| unit: Unit | str | DefaultUnit | _NoArgProvided = _no_arg_provided, | ||
| dtype: DTypeLike | _NoArgProvided = _no_arg_provided, | ||
| with_variances: bool | _NoArgProvided = _no_arg_provided, |
There was a problem hiding this comment.
The whole _NoArgProvided thing looks strange but it's done that way to distinguish the case when no argument was provided by the user from the case when the user provided the default argument value.
We need to distinguish the cases because if the user explicitly provided the default argument value we should use that. But if the argument was not provided then we will determine it from obj.
| class _NoArgProvided: | ||
| '''Dummy type to indicate that no argument was passed to the function.''' | ||
|
|
||
| pass | ||
|
|
||
|
|
||
| _no_arg_provided = _NoArgProvided() |
There was a problem hiding this comment.
| class _NoArgProvided: | |
| '''Dummy type to indicate that no argument was passed to the function.''' | |
| pass | |
| _no_arg_provided = _NoArgProvided() | |
| _no_arg_provided = object() |
Should have the same effect but simpler?
There was a problem hiding this comment.
Yeah I agree it's simpler, but how do you type annotate the arguments if you do that?
One option is to keep the type annotations from the original functions (sc.zeros etc) and then ignore the type errors. 🤔 Maybe that's better actually, because changing the type annotations from the original functions might be confusing.
There was a problem hiding this comment.
Hmm, you are right, I think we want it in the type annotation?
|
@SimonHeybrock is there anything blocking you from approving this PR? |
Fixes #3466