feat(JAQPOT-127): jaqpotpy-datasets-refactoring #28
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
All test datasets should be concentrated at a single point and not in several subfolders within jaqpotpy
This file is going to be deleted once the new classes are up and running
This file is going to be deleted once the new dataset classes are ready
periklis91
changed the title
This class was completely redundant
All commints after this commit will fix bugs
There was an error when the user was providing a list of a single string. Now it is fixed
Instead of taking different scenarios, just drop smiles into a list and work with this throughout the code
This consists of the first round of tests for dataset construction. Further tests need to be written, this is a start
Added a test that checks if a TypeError is raised in the case of the user providing both a path and a csv
Change all references of SmilesDataset and MolecularTabularDataset to JaqpotpyDataset
Removed the wildcard import from datasets.__inits__
doa_m was the assigned to doa.fit() so this was renamed accordingly
The changes on JaqpotpyDataset had created several compatibilty issues with the classes MolecularModel() and MolecularSKLearn which are now fixed. Next thing to fix is the .predict() method and also to checkout what is going on with the onnx framework and the predictions
Previously, once a preprocessor was defined, preprocessing of X and y were considered class attributes instead of object attributes, so no changes could be made. Now preprocessing of X and y are object attributes and as such are initialised and can be reset with each new object initilisation
models are now compatible with the new JaqpotpyDataset
vassilismin
reviewed
Jun 28, 2024
|
|
||
| @property | ||
| def task(self): | ||
| return self._task | ||
|
|
||
| @task.setter | ||
| def task(self, value): | ||
| if value is None: |
There was a problem hiding this comment.
Suggesting to reduce to a single if statement
if value is None or value.lower() not in ['regression', 'classification']:
vassilismin
approved these changes
Jun 28, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This branch does not only involve testing of datasets but also refactoring of the datasets class schema and classes themselves. Once all necessary changes are implemented, I will assign someone to review them