Dtype str in desc #438
Dtype str in desc #438
Conversation
|
@coretl @genematx @mrakitin @danielballan First pass at this is done, and should be ready for review. All tests are passing now. |
|
The linter error appears to be related to |
|
Lint runs on the result of the branch merged to main, so maybe that made some linting error? Please can you update |
| @@ -71,6 +71,7 @@ async def _describe(self) -> Dict[str, DataKey]: | |||
| source=self.panda_device.data.hdf_directory.source, | |||
| shape=ds.shape, | |||
| dtype="array" if ds.shape != [1] else "number", | |||
| dtype_str="<f8", # PandA data should always be written as Float64 | |||
There was a problem hiding this comment.
This will be true after PandABlocks/PandABlocks-client#87
There was a problem hiding this comment.
@coretl, are you OK with keeping this change merge the PR?
There was a problem hiding this comment.
Looking good. Thanks, Jakub! I have one potential refactoring comment, but likely it's just may lack of understanding of internals of ophyd-async.
Aside, are we going to add chunks field as well? I think it should go into a separate PR, but we will need them in Tiled eventually.
| dtype_numpy = "" | ||
| if isinstance(value, list): | ||
| if len(value) > 0: | ||
| dtype_numpy = np.dtype(type(value)).descr[0][1] |
There was a problem hiding this comment.
I'm not sure I understand how this works. Should it be np.dtype(value[0].dtype) on line 81 (or something along those lines). Also, is value always either a scalar or a list? Would it be safer to check if it's an iterable instead?
Or is it a numpy array or a list?
Maybe adding a typehint to value in the function signature could be helpful.
There was a problem hiding this comment.
Good points, @genematx! I overlooked that case during my review.
Maybe it should just be np.dtype(type(value[0])) as the value[0] will be a number, which won't (or may not?) have .dtype method.
To compare two approaches:
In [28]: np.dtype(type([1, 2, 3][0])).descr
Out[28]: [('', '<i8')]
In [29]: np.dtype(type([1, 2, 3])).descr
Out[29]: [('', '|O')]I support adding a type annotation to the signature and a docstring to explain the expected input types.
…st for `SoftArrayConverter`
Solves #257.
Currently a work in progress - the tests don't yet pass. Making a draft PR to track progress.