Skip to content
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

Serializable cfg entries #392

Closed
tensionhead opened this issue Dec 13, 2022 · 3 comments · Fixed by #393
Closed

Serializable cfg entries #392

tensionhead opened this issue Dec 13, 2022 · 3 comments · Fixed by #393
Assignees
Labels
Bug An error that is serious but does not break (parts of) the package. However, it clearly impedes the

Comments

@tensionhead
Copy link
Contributor

As reported by @kajal5888, if a frontend parameter (e.g. foilim) is set via a non-serializable datatype (like numpy's int64) the standard spy.save fails because the cfg dictionary can't be dumped in to the accompanying json. Easiest to reproduce is sth like this:

my_data = ...  # some AnalogData
cfg = spy.StructDict()                                                                                                                           
cfg.foilim = [80, np.int64(120)]

spec = spy.freqanalysis(my_data, cfg)

spec.save('test')

this fails with

TypeError: Object of type int64 is not JSON serializable

How to Fix Idea

When updating the .cfg property of a Syncopy data object (for replayabilty) inside a frontend, it should be checked that all values are serializable, and if possible casted to appropriate types. So np.int64 -> int, np.float64 -> float and so on

@tensionhead tensionhead added the Bug An error that is serious but does not break (parts of) the package. However, it clearly impedes the label Dec 13, 2022
@kajal5888 kajal5888 linked a pull request Dec 13, 2022 that will close this issue
15 tasks
@dfsp-spirit
Copy link
Collaborator

dfsp-spirit commented Dec 14, 2022

On the PR: This could actually be relevant in more places than just the foilim, maybe we want more general checks, as mentioned by @tensionhead.

Another question is whether we want to reject or try to automatically convert numpy datatypes as input in cases when they would go into the cfg. I think converting would be friendly, as using numpy datatypes as input seems a natural thing to do in some cases.

@tensionhead
Copy link
Contributor Author

Yes, afaik all the numpy arrays we do accept in the frontend are of 'the sequence type', e.i. 1d-arrays. For those calling .tolist(() before casting to a serializable data type should be straightforward.

@tensionhead
Copy link
Contributor Author

Got done with #407

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug An error that is serious but does not break (parts of) the package. However, it clearly impedes the
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants