-
Notifications
You must be signed in to change notification settings - Fork 78
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
Implementation for discrete multisession #135
Implementation for discrete multisession #135
Conversation
Thank you for your contribution. We require contributors to sign our Contributor License Agreement (CLA). We do not have a signed CLA on file for you. In order for us to review and merge your code, please sign our CLA here. After you signed, you can comment on this PR with |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added two small remarks, but in general this looks promising. Will start running tests, and then proceed with a more in-depth review.
Hi @introspective-swallow , thanks a lot for your contribution! I skimmed the code and it looks good overall, made two small comments. I just started to run tests. I'll do another in-depth pass through the code in the next days. |
Also, if you don't mind, do you have an example of a fitted embedding that you could add to the PR description as a reference? |
@introspective-swallow a few tests are failing, could you have a look? I created another PR #136 as some of the failing tests are unrelated to your contribution. I will fix these independently, but others are failing due to the test setup in this PR. Let me know if you have questions about the particular tests. You can also run them locally using |
I do have one but the data is private for now though it will be published soon. I could remove the label names and include a fitted embedding, would that work? After it is published we could share it to make a demo as well. |
Thank you for your contribution. We require contributors to sign our Contributor License Agreement (CLA). We do not have a signed CLA on file for you. In order for us to review and merge your code, please sign our CLA here. After you signed, you can comment on this PR with |
Yes! I fixed and added some tests and now pytest is giving me no errors. |
Perfect. In the meantime I finished #136 to resolve the upstream errors unrelated to this PR. I will get this merged, then update your PR, and confirm that all tests build.
That sounds great, thanks a lot! |
Thank you for your contribution. We require contributors to sign our Contributor License Agreement (CLA). We do not have a signed CLA on file for you. In order for us to review and merge your code, please sign our CLA here. After you signed, you can comment on this PR with |
Added the changes from #136 to see if tests pass now. |
@cla-bot check |
Thank you for your contribution. We require contributors to sign our Contributor License Agreement (CLA). We do not have a signed CLA on file for you. In order for us to review and merge your code, please sign our CLA here. After you signed, you can comment on this PR with |
Thanks for tagging me. I looked for a signed form under your signature again, and updated the status on this PR. If the check was successful, no further action is needed. If the check was unsuccessful, please see the instructions in my first comment. |
@stes I already filled the CLA and after triggering a check it doesn't seem to work :( |
@introspective-swallow just checked, you filled in the URL to your full github username (https://github.com/introspective-swallow). you need to correct this to only your username, then it will work. |
@cla-bot check |
Thanks for tagging me. I looked for a signed form under your signature again, and updated the status on this PR. If the check was successful, no further action is needed. If the check was unsuccessful, please see the instructions in my first comment. |
fixed! cc @introspective-swallow |
Thank you! My mistake. |
No worries! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm; I did not run it though; just overall code review. @stes can final merge confirm here
only last issue is to edit the docs to say how to use it. thanks again @introspective-swallow -- sorry for the delay in reviewing this! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm! thanks again for this contribution 💪
old spelling error, caught by new tests :)
- spelling
- spelling
one minor doc issue to fix (I can do so tomorrow!): _____________________________ [doctest] usage.rst ______________________________
1039 num_batches=5)
1040
1041 # multi-session
1042 multi_score = cebra.sklearn.metrics.infonce_loss(multi_cebra_model,
1043 neural_session1,
1044 continuous_label1,
1045 session_id=0,
1046 num_batches=5)
1047
1048
UNEXPECTED EXCEPTION: ValueError('Labels invalid: must have the same type of features as the ones used for fitting,expects int64, got float64.')
Traceback (most recent call last):
File "/opt/hostedtoolcache/Python/3.8.18/x64/lib/python3.8/site-packages/pytest_sphinx.py", line 366, in _DocTestRunner__run
exec(
File "<doctest usage.rst[33]>", line 9, in <module>
File "/home/runner/work/CEBRA/CEBRA/cebra/integrations/sklearn/metrics.py", line 87, in infonce_loss
cebra_model._check_labels_types(y, session_id=session_id)
File "/home/runner/work/CEBRA/CEBRA/cebra/integrations/sklearn/cebra.py", line [86](https://github.com/AdaptiveMotorControlLab/CEBRA/actions/runs/9258095298/job/25467946597?pr=135#step:10:87)3, in _check_labels_types
raise ValueError(
ValueError: Labels invalid: must have the same type of features as the ones used for fitting,expects int64, got float64.
usage.rst:1048: UnexpectedException |
will will fix the rest of the issues in this issue, as it's related to matplotlib and not this PR: #150 |
9358820
into
AdaptiveMotorControlLab:main
I implemented the discrete multisession mode for CEBRA, analogous to the continuous multisession mode. It works for one discrete label and uses the uniform distribution for prior sampling. I also included it in the sklearn integration. I added tests for the sampler, loader, solver and the sklearn integration.
Added objects that differ from the continuous multisession are the loader
DiscreteMultiSessionDataLoader
and the samplerDiscreteMultisessionSampler
.As an example, this are session embeddings fitted with discrete multisession for the data I am working with (that will be published soon):
I am new to open source contribution, so I'm excited to get your corrections and feedback!