-
Notifications
You must be signed in to change notification settings - Fork 130
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
Integrate get_molnet_dataset with Splitter #209
Integrate get_molnet_dataset with Splitter #209
Conversation
Codecov Report
@@ Coverage Diff @@
## master #209 +/- ##
==========================================
- Coverage 82.32% 81.59% -0.74%
==========================================
Files 106 106
Lines 4974 5026 +52
==========================================
+ Hits 4095 4101 +6
- Misses 879 925 +46 |
frac_test=.1, seed=777, return_smiles=False, | ||
target_index=None): | ||
target_index=None, task_index=0, **kwargs): |
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.
Can you add docstring? or want to do in other PR?
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.
done
if isinstance(split, str): | ||
splitter = split_method_dict[split]() | ||
elif isinstance(split, BaseSplitter): | ||
splitter = split |
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 feel adding else: raise TypeError('message')
is friendly.
if isinstance(splitter, ScaffoldSplitter): | ||
get_smiles = True | ||
else: | ||
get_smiles = return_smiles |
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 feel adding else: raise TypeError('message')
is friendly.
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 think TypeError
is not necessary for this line.
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 want to fix the message to show the "actual" type.
@@ -7,8 +7,9 @@ | |||
from chainer_chemistry.datasets import NumpyTupleDataset | |||
from chainer_chemistry.datasets import molnet | |||
|
|||
expect_bbbp_lengths = [1631, 203, 205] | |||
expect_bbbp_lengths = [1633, 203, 203] |
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.
Do you know why this happen? Rdkit version??
If rdkit version issue, I want to fix recommended rdkit version for each chainer chemistry version.
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.
It seems that this change is caused by the difference between scaffold split and random split. But unexpected.
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 think this is not "unexpected" since you implement that StratifiedSplitter to have much "train" data for the remainder, right?
Fix. |
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.
Ok
frac_test=.1, seed=777, return_smiles=False, | ||
target_index=None): | ||
target_index=None, task_index=0, **kwargs): |
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.
done
if isinstance(splitter, ScaffoldSplitter): | ||
get_smiles = True | ||
else: | ||
get_smiles = return_smiles |
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 want to fix the message to show the "actual" type.
@@ -7,8 +7,9 @@ | |||
from chainer_chemistry.datasets import NumpyTupleDataset | |||
from chainer_chemistry.datasets import molnet | |||
|
|||
expect_bbbp_lengths = [1631, 203, 205] | |||
expect_bbbp_lengths = [1633, 203, 203] |
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 think this is not "unexpected" since you implement that StratifiedSplitter to have much "train" data for the remainder, right?
I want to fix some document and message, but I think it is faster to make PR instead of asking you to fix. |
Please merge after #201 #202