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.

Sign up for GitHub

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

Jump to bottom

Set default offset to an Offset object #180

Merged
merged 5 commits into from
Oct 27, 2024
Merged

Set default offset to an Offset object #180

merged 5 commits into from
Oct 27, 2024

Conversation

icarosadero
Copy link
Contributor

@icarosadero icarosadero commented Oct 16, 2024 •
edited by stes
Loading

Should fix #174

@cla-bot cla-bot
Copy link

cla-bot bot commented Oct 16, 2024

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 @cla-bot check to trigger another check.

@icarosadero
Copy link
Contributor Author

@cla-bot check

@cla-bot cla-bot bot added the CLA signed label Oct 16, 2024
@cla-bot cla-bot
Copy link

cla-bot bot commented Oct 16, 2024

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.

@icarosadero icarosadero mentioned this pull request Oct 16, 2024
Closed
@MMathisLab MMathisLab requested a review from stes October 17, 2024 03:08
@stes stes mentioned this pull request Oct 20, 2024
Merged
stes
stes requested changes Oct 26, 2024
View reviewed changes
Copy link
Member

@stes stes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@icarosadero i think the offset here is actually not used, right? We could also consider deprecating the argument instead.

Do you have a code example that's fixed by this change?

stes
stes reviewed Oct 26, 2024
View reviewed changes
Copy link
Member

@stes stes left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

update: the original PR description didnt link the correct issue, i see that #174 is actually a problem and this is the correct fix

stes
stes reviewed Oct 26, 2024
View reviewed changes
cebra/data/datasets.py Outdated Show resolved Hide resolved
@stes
Copy link
Member

stes commented Oct 26, 2024

can be merged once tests pass

stes
stes requested changes Oct 26, 2024
View reviewed changes
Copy link
Member

@stes stes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@icarosadero if you get the chance, could you add a test that reproduces this error #174 (comment) to this PR?

@stes stes added the 🐞 fix label Oct 26, 2024
stes
stes approved these changes Oct 27, 2024
View reviewed changes
Copy link
Member

@stes stes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your contribution, @icarosadero , lgtm!

@stes stes merged commit 51f048d into AdaptiveMotorControlLab:main Oct 27, 2024
11 checks passed
@stes stes mentioned this pull request Oct 27, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stes stes stes approved these changes

Assignees
No one assigned
Labels
CLA signed 🐞 fix
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

TensorDataset.offset is set to an int but Dataset.offset is set to an Offset
3 participants
@icarosadero @stes @MMathisLab