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

Selecting train IDs in DataCollection and SourceData #559

Merged
merged 6 commits into from
Oct 23, 2024
Merged

Selecting train IDs in DataCollection and SourceData #559

merged 6 commits into from
Oct 23, 2024

Conversation

tmichela
Copy link
Member

@tmichela tmichela commented Oct 4, 2024

Allow using the object[] syntax to select train IDs in addition to source or keys for DataCollection and SourceData

github-advanced-security[bot]
github-advanced-security bot found potential problems Oct 4, 2024
View reviewed changes
extra_data/tests/test_reader_mockdata.py Dismissed Show dismissed Hide dismissed
@tmichela tmichela requested a review from fadybishara October 18, 2024 11:05
@fadybishara
Copy link
Contributor

I have a minor comment (see review), otherwise everything LGTM! The RTD should also be updated, right?

fadybishara
fadybishara approved these changes Oct 21, 2024
View reviewed changes
Copy link
Contributor

@fadybishara fadybishara left a comment

Choose a reason for hiding this comment

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

Modulo the minor comment, LGTM!

extra_data/reader.py Show resolved Hide resolved
extra_data/tests/test_sourcedata.py Show resolved Hide resolved
@tmichela
Copy link
Member Author

I added a few lines in the docs, will merge in a few days (when Thomas is back) if no-one objects the hybrid behavior.

@takluyver
Copy link
Member

Thanks, the changes LGTM too (and I lean towards agreeing with Philipp - it seems worth blocking a run[source, index] selection to make clearer errors on simple mistakes)

@tmichela
Copy link
Member Author

Thanks all for the review!

@tmichela tmichela merged commit fd2370c into master Oct 23, 2024
8 checks passed
@takluyver takluyver added this to the 1.19 milestone Nov 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@philsmt philsmt philsmt left review comments

@fadybishara fadybishara fadybishara approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
1.19
Development

Successfully merging this pull request may close these issues.
4 participants
@tmichela @fadybishara @takluyver @philsmt