-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Modifications to Image Loader #3695
Conversation
@rbharath Please Review |
@aaronrockmenezes please use Yapf version 0.32.0 for python linting |
This seems to be causing FAILED deepchem/data/tests/test_image_loader.py::TestImageLoader::test_multitype_zip_load - assert (2,) == (2, 768, 1024, 3)
Right contains 3 more items, first extra item: 768
Full diff:
- (2, 768, 1024, 3)
+ (2,) in python 3.8 unit tests |
@shreyasvinaya It seems like the a_image.tif is of a different dimension, (330, 44) and that doesn't match with the .png image for the test. Could that be the issue? |
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.
This looks good to me, the image loader test that was failing in python 3.10 is also now fixed
@rbharath Please do a final review |
for subfile in os.listdir(label_file) | ||
] | ||
remainder += dirfiles | ||
elif extension == ".zip": |
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.
@aaronrockmenezes Do zip files have a set ordering upon extraction? The example above uses a label zip and a data directory zip. Do we know ordering is preserved or is it implementation dependent and can change?
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.
All files within a zipped folder are extracted in the order they appear in the directory that is to be extracted. So yes, as far as I know, the order is maintained.
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.
@aaronrockmenezes This looks mostly good but have a question about zip file handling below
@rbharath Yes, the zip file will be extracted in the same order as they appear in the directory that is to be extracted. |
@aaronrockmenezes Can you add a unit test that verifies this? Have sample directory, zip then use loader and verify it works correctly. Once that's added we can merge in |
@rbharath Added test to verify that the order of contents is maintained. Please review |
# These are the known dimensions of face.png | ||
assert dataset.X.shape == (1, 768, 1024, 3) | ||
assert dataset.y.shape == (1, 768, 1024, 3) | ||
|
||
def test_tif_simple_load(self): | ||
loader = dc.data.ImageLoader() | ||
dataset = loader.create_dataset(self.tif_image_path) | ||
# TODO(rbharath): Where are the color channels? |
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.
@rbharath do we remove this comment?
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.
Yes, we should but fine to do in follow up 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.
LGTM
Description
Image Loader can now be used to load images as labels.
[Issues: Sorting using sorted() is causing problems due to lexographic sorting, this is not what a normal OS does, leading to mismatch between labels and data].
Type of change
Please check the option that is related to your PR.
Checklist
yapf -i <modified file>
and check no errors (yapf version must be 0.32.0)mypy -p deepchem
and check no errorsflake8 <modified file> --count
and check no errorspython -m doctest <modified file>
and check no errors