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

Changing resolution of train/test/validation indices in datamanager. And make data manager mypy compliant #19

Merged
merged 17 commits into from
Nov 24, 2022

Conversation

ojpb
Copy link
Contributor

@ojpb ojpb commented Sep 9, 2022

This PR simplifies the resolution of the train/test/validation indices in the datamanager. An image has been added to the doc string to explain this. Tests for the resolution of indices for all cases have also been added.

@ojpb ojpb requested a review from thomasgaudelet September 9, 2022 13:18
@alicerdv alicerdv changed the title adding mypy and fixing data_manger.py with mypy Changing resolution of train/test/validation indices in datamanager. And make data manager mypy compliant Nov 15, 2022
thomasgaudelet
thomasgaudelet previously approved these changes Nov 22, 2022
Copy link
Collaborator

@a-pouplin a-pouplin left a comment

Choose a reason for hiding this comment

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

It seems much easier to use this way :)
If there is no remaining train or test indices, should it raise an error ?

@alicerdv
Copy link
Contributor

It seems much easier to use this way :)
If there is no remaining train or test indices, should it raise an error ?

Good point. I will add that

@alicerdv
Copy link
Contributor

It seems much easier to use this way :)
If there is no remaining train or test indices, should it raise an error ?

I've now added an error when test indices are none after train/test are made :) (there was already an error for train indices being none)

a-pouplin
a-pouplin previously approved these changes Nov 23, 2022
@alicerdv alicerdv merged commit 3a2fdb3 into RelationRx:main Nov 24, 2022
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.

4 participants