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

Refactor effdet dataloaders wrapper and fix the image sizes passed to effdet #630

Merged
merged 11 commits into from Feb 10, 2021

Conversation

potipot
Copy link
Contributor

@potipot potipot commented Jan 30, 2021

I started working on a refactor of effdet dataloaders. I noticed many common parts in the build_train_batch, build_valid_batch and build_infer_batch and came up with an idea to unify this API. There is a main loop over all records in order to extract certain information about the image tensor: image itself, bboxes and labels, image sizes and scales. Whether we want to extract certain parameters depends on the train/valid/infer part.

Main change is the process_record function that will be used in single iteration over all records (instead of doing it twice now, second time if we need to extract additional info not handled by the base build_train_batch function).

I also removed the tensor casting from the process_record function in order to improve code readability. The idea is to handle to_tensor casting by collate_records.

This is a draft PR. Looking forward for your thoughts and suggestions!

@potipot potipot changed the title Refactor effdet datalaoders wrapper and fix the image sizes passed to effdet Refactor effdet dataloaders wrapper and fix the image sizes passed to effdet Jan 30, 2021
@potipot potipot marked this pull request as ready for review February 2, 2021 15:19
@codecov
Copy link

codecov bot commented Feb 2, 2021

Codecov Report

Merging #630 (70bb07d) into master (47c77b8) will increase coverage by 0.04%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #630      +/-   ##
==========================================
+ Coverage   85.63%   85.68%   +0.04%     
==========================================
  Files         149      149              
  Lines        3245     3242       -3     
==========================================
- Hits         2779     2778       -1     
+ Misses        466      464       -2     
Flag Coverage Δ
unittests 85.68% <100.00%> (+0.04%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
icevision/models/ross/efficientdet/dataloaders.py 100.00% <100.00%> (+4.76%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 47c77b8...70bb07d. Read the comment docs.

@lgvaz lgvaz linked an issue Feb 2, 2021 that may be closed by this pull request
@lgvaz
Copy link
Collaborator

lgvaz commented Feb 5, 2021

I'll take a look at this today =)

@lgvaz lgvaz self-requested a review February 5, 2021 14:13
Copy link
Collaborator

@lgvaz lgvaz left a comment

Choose a reason for hiding this comment

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

I really really like these changes!!! I just suggested some very small changes.

Thanks a lot Pawel!

icevision/models/ross/efficientdet/dataloaders.py Outdated Show resolved Hide resolved
icevision/models/ross/efficientdet/dataloaders.py Outdated Show resolved Hide resolved
icevision/models/ross/efficientdet/dataloaders.py Outdated Show resolved Hide resolved
icevision/models/ross/efficientdet/dataloaders.py Outdated Show resolved Hide resolved
icevision/models/ross/efficientdet/dataloaders.py Outdated Show resolved Hide resolved
icevision/models/ross/efficientdet/dataloaders.py Outdated Show resolved Hide resolved
potipot and others added 2 commits February 5, 2021 19:54
@potipot
Copy link
Contributor Author

potipot commented Feb 5, 2021

So as I changed Tensor to tensor invocations, the effdet tests complain that the type is wrong, "should be tf.float32". Changing it back to what it was passes the tests. Don't know yet why this is happening

@potipot potipot requested a review from lgvaz February 7, 2021 15:44
icevision/models/ross/efficientdet/dataloaders.py Outdated Show resolved Hide resolved
targets["bbox"].append(bboxes)
# convert to tensors
batch_images = torch.stack(batch_images)
batch_bboxes = [tensor(bboxes, dtype=torch.float32) for bboxes in batch_bboxes]
Copy link
Collaborator

Choose a reason for hiding this comment

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

It's a bit more verbose, but I like the fact that we're passing dtype explicitly here, specially because as you pointed out effdet complains if the dtype is not what it expects

@potipot
Copy link
Contributor Author

potipot commented Feb 8, 2021

Oh this missing space in COCOMetric test keeps popping up, its really annoying, sometimes it passes others it doesn't. Any idea how to fix this for good?

@lgvaz
Copy link
Collaborator

lgvaz commented Feb 8, 2021

I know what this is related to, the original pycocotools has the space. Because of mmdet we started using their fork mmpycocotools and this one doesn't have spaces

The tests are done using mmpycocotools, do you by any change have pycocotools locally?


Again, pycocotools causing problems -.-'

Copy link
Collaborator

@lgvaz lgvaz left a comment

Choose a reason for hiding this comment

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

latest change removing commas looks good!

@potipot
Copy link
Contributor Author

potipot commented Feb 9, 2021

Yes, I do have it locally. Waiting for resolution of those errors before merge?

@lgvaz
Copy link
Collaborator

lgvaz commented Feb 9, 2021

Yes, I do have it locally
pycocotools or mmpycocotools?

For the errors just remove the spaces

@potipot
Copy link
Contributor Author

potipot commented Feb 10, 2021

I think we are set.

Copy link
Collaborator

@lgvaz lgvaz left a comment

Choose a reason for hiding this comment

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

Perfect!! We are all set indeed!! let's merge

@lgvaz lgvaz merged commit e60e9af into airctic:master Feb 10, 2021
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.

EfficientDet pretrained arch transfer learning
2 participants