Skip to content

Record.convert_dtype should fail if expected data is missing #343

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

Merged
merged 1 commit into from
Mar 14, 2022

Conversation

bemoody
Copy link
Collaborator

@bemoody bemoody commented Mar 11, 2022

This is not directly related to other issues surrounding smooth_frames, but a minor bug that probably doesn't affect anyone at present.

Record.convert_dtype can be used to change the storage format of the signal array(s). (It's unlikely that applications will ever use this function directly and it's not included in the package documentation.)

However, if convert_dtype is called with physical=True and smooth_frames=False, it should convert the data type(s) of e_p_signal. It shouldn't try to modify p_signal in this case (and the previous code would not have worked correctly either.) If you ask to convert the data type of e_p_signal, and e_p_signal is None, this should raise an exception.

If physical is true, and smooth_frames is false, and e_p_signal is
None, this function should raise an exception.  It should not silently
modify p_signal instead.

The previous code didn't make any sense: p_signal is a numpy array, so
you can't change its data type by assigning to a slice of it, and even
if that conversion were possible, the slicing was being done along the
wrong axis.  This reverts a dubious change in commit
725e7a6, which seems unrelated to the
rest of that commit.
@bemoody
Copy link
Collaborator Author

bemoody commented Mar 11, 2022

(pull #244 is where this was introduced)

@tompollard
Copy link
Member

Looks good to me, thanks.

@tompollard tompollard merged commit 169f25d into master Mar 14, 2022
@tompollard tompollard deleted the convert-dtype-bug branch March 14, 2022 14:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants