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

Pydantic validation settings #329

Merged
merged 53 commits into from
Jun 17, 2024
Merged

Pydantic validation settings #329

merged 53 commits into from
Jun 17, 2024

Conversation

timonmerk
Copy link
Contributor

@toni-neurosc Sorry for opening this PR so soon, but I had a look at this branch and for the most part I can understand it.
Would it be possible to adapt one example from the examples folder s.t. I could follow along and also help adapt the other examples / tests?

@toni-neurosc
Copy link
Collaborator

No problem at all, I'll get to it after lunch, the tests/examples were basically the next thing in line, I think I should be able to finish the bulk of it today

@toni-neurosc toni-neurosc force-pushed the main branch 3 times, most recently from bc837c6 to 0a75c38 Compare June 4, 2024 12:15
@timonmerk
Copy link
Contributor Author

Hey @toni-neurosc, thanks so much for those changes. I saw you now already added some burst addition in this PR. Sorry for messing with that.

I now did a merge that resulted with conflicts, but I wanted to push my additions in nm_bursts as well to this branch.

There are some points I want to add. I looked at all the points that you marked with "TONI". There is one test currently failing: test_osc_features:test_fft_wrong_logtransform_param_init.
Is there in Pydantic a method to also check after initialization that fields will be validated? I thought that model_validator(mode="after") would relate to that point. And actually you set this as a decorator in NMSettings.validate_settings, so I am not sure why this one doesn't catch it...

Regarding your comment about zero divisions, there is actually a test that checks for all values being zero or NaN in test_nan_values.py. So in this case there might be warnings, especially when zero values are log-transformed (e.g. in nm_oscillatory) which results in -np.inf. But this should be expected behavior.

There was also a bug (I think) in NMSettings.enable_all_features, I changed it now that all features are enabled and not disabled.

Apart from that, the changes are great. I would also start adapting the docstrings now that they match the params for each function.

@timonmerk
Copy link
Contributor Author

Hey @toni-neurosc, I am getting an error with your optimized sharpwave module import:

from numpy.core._methods import _mean as np_mean
ModuleNotFoundError: No module named 'numpy.core._methods'

Could that be again an OS-specific thing?

@toni-neurosc
Copy link
Collaborator

toni-neurosc commented Jun 16, 2024

Hey @toni-neurosc, I am getting an error with your optimized sharpwave module import:

from numpy.core._methods import _mean as np_mean
ModuleNotFoundError: No module named 'numpy.core._methods'

Could that be again an OS-specific thing?

Well no, what happened is that Numpy 2.0.0 release just 4 hours ago and that's what pip installed on the test server xD Weird coincidence. Probably we wouldn't even have noticed if I hadn't decided to use a numpy internal function to squeeze some performance lol. I'm gonna update Numpy on my local and fix it.

EDIT: found the relevant change, this caused the error: https://numpy.org/devdocs/numpy_2_0_migration_guide.html#numpy-core-namespace

@toni-neurosc
Copy link
Collaborator

Ok, @timonmerk , I fixed the Numpy issue, it was nothing, they just changed the name of the internal module files with the version change, no big deal, basically added a couple of underscore and it's working again.

Also a couple more changes:

  • Commit a2da5cb: I added verbose=False to the MNE connectivity classes so that it does not output to the console.
  • Commit a500e01: I dealt with the RuntimeError of 0/0 division that was being thrown by Numpy, it was not because of input data normalization (it's disabled by default in the settings) but because of feature normalization sometimes having 0 mean and variance. I basically did as sklearn does and when std=0 I replaced it with std=1. Could have also used sklearn StandardScaler but it's actually twice as slow.
  • Commit c003339: merged the latest changes in main to this branch, just a bit of housekeeping

@toni-neurosc
Copy link
Collaborator

Hey @toni-neurosc, the concept of temporal waveform shape is overall very little used in the invasive electrophysiology community. Therefore, with this example we aimed to present the overall concept behind waveform shape, what characteristics can be computed, how they relate to movement, and also how those can be used for decoding.

I know that it used quite a "hacky" way of presenting the concept, and probably a better way is to do the convolution, peak finding etc. in the example itself without having the dependencies to the nm_sharpwave code.

So it would be great if you could also adapt the example to show the similar functionality with the speed improvements. I really appreciate the performance increase, but I definitely want to keep the example in the documentation for explainability.

Ok, I will try to rework the example and maybe implement some functions to expose the result of calc_feature without having to basically manually run the steps of calc_feature in the example itself.

@timonmerk
Copy link
Contributor Author

@toni-neurosc, I've fixed now the sharpwave example. It was much simpler than expected.
I also had to adapt some dependencies, such that the numpy.core functions could be imported.

I also fixed the add feature example that it passes the automatic test, and added a function in nm_IO to read raw_data. This fixes #304.

So feel free to add the method adding a feature in a better way after stream initialization.
Otherwise you can merge this PR. Many thanks again for the crazy amount of work adapting basically the whole package!

py_neuromodulation/nm_features.py Outdated Show resolved Hide resolved
py_neuromodulation/nm_features.py Outdated Show resolved Hide resolved
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