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

address tuple in collate #336

Closed
wants to merge 1 commit into from
Closed

address tuple in collate #336

wants to merge 1 commit into from

Conversation

michalozeryflato
Copy link
Collaborator

No description provided.

Copy link
Collaborator

@YoelShoshan YoelShoshan left a comment

Choose a reason for hiding this comment

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

LGTM

I do hope, however, that it doesn't break any expected behavior.
Because until this change, I think that if people wanted collate to not touch their sequential, they could have used a tuple to express that (without using any additional instructions to the collate function).

Copy link
Collaborator

@mosheraboh mosheraboh left a comment

Choose a reason for hiding this comment

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

Thanks Michal.
It will break compatibility, I suggest to make it a custom function (so you can force it per key).
If you are ok with it, lets talk and I'll share more details.

@michalozeryflato
Copy link
Collaborator Author

I accepted the reviewers' comments and fixed the collate in bmfm-bio data modules. closing this PR.

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.

3 participants