Skip to content
This repository has been archived by the owner on Nov 17, 2023. It is now read-only.

[MXNET-664] Support integer type in ImageIter #11864

Merged
merged 1 commit into from
Jul 26, 2018
Merged

[MXNET-664] Support integer type in ImageIter #11864

merged 1 commit into from
Jul 26, 2018

Conversation

vandanavk
Copy link
Contributor

@vandanavk vandanavk commented Jul 24, 2018 •
edited
Loading

Description

Support int32 type for labels in ImageIter. By default, labels are of type float and therefore lead to precision issues.

Checklist

Essentials

  • The PR title starts with [MXNET-664],
  • Changes are complete (i.e. I finished coding on this PR)
  • All changes have test coverage:
  • Unit tests are added for small changes to verify correctness (e.g. adding a new operator)
  • Code is well-documented:
  • For user-facing API changes, API doc string has been updated.
  • To the my best knowledge, examples are not affected by this change

Changes

  • Added a new parameter to ImageIter that specifies the dtype of the label. By default, it is set to float32, which was the case in the old code.
  • Modified test_imageiter() unit test to test the new parameter with int32, float32, int64, float64.

Comments

  • RecordIO and ImageDetIter have not been changed.
  • The example (profiler_imageiter.py) is unaffected

@vandanavk vandanavk requested a review from szha as a code owner July 24, 2018 00:06
haojin2
haojin2 reviewed Jul 24, 2018
View reviewed changes
tests/python/unittest/test_image.py Outdated
path_root='')
for batch in test_iter:
pass
for dtype in ['int32', 'float32']:
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this only support int32 & float32? If possible please also check float64 and int64.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, I can check float64 and int64

Copy link
Contributor

Choose a reason for hiding this comment

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

If that's the case then please also update the doc string above correspondingly.

@vandanavk
Copy link
Contributor Author

@sandeep-krishnamurthy @apeforest @Roshrini

apeforest
apeforest suggested changes Jul 24, 2018
View reviewed changes
Copy link
Contributor

@apeforest apeforest left a comment

Choose a reason for hiding this comment

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

Looks good overall. Left two small comments.

python/mxnet/image/image.py Outdated
@@ -1091,7 +1093,7 @@ def __init__(self, batch_size, data_shape, label_width=1,
imgkeys = []
for line in iter(fin.readline, ''):
line = line.strip().split('\t')
label = nd.array([float(i) for i in line[1:-1]])
label = nd.array([i for i in line[1:-1]], dtype=dtype)
Copy link
Contributor

Choose a reason for hiding this comment

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

[i for i in line[1:-1]] is just line[1:-1], right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correct. made the change

tests/python/unittest/test_image.py Outdated
test_iter = mx.image.ImageIter(2, (3, 224, 224), label_width=1, imglist=im_list,
path_root='')
for _ in range(3):
def check_ImageIter(dtype='float32'):
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: use lower case imageiter to be consistent with test_imageiter

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@vandanavk vandanavk force-pushed the python_bugs branch 2 times, most recently from 44023ad to 156d793 Compare July 24, 2018 19:44
@vandanavk vandanavk mentioned this pull request Jul 24, 2018
Closed
sandeep-krishnamurthy
sandeep-krishnamurthy reviewed Jul 24, 2018
View reviewed changes
python/mxnet/image/image.py
@@ -1091,7 +1093,7 @@ def __init__(self, batch_size, data_shape, label_width=1,
imgkeys = []
for line in iter(fin.readline, ''):
line = line.strip().split('\t')
label = nd.array([float(i) for i in line[1:-1]])
label = nd.array(line[1:-1], dtype=dtype)
Copy link
Contributor

Choose a reason for hiding this comment

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

What if I pass dtype='int8' or something that is not supported?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

int8 specifically doesn't cause any issue. but yes, generally, a check is required. Adding an assert for the next iteration. Adding a test for the default case too.

sandeep-krishnamurthy
sandeep-krishnamurthy suggested changes Jul 24, 2018
View reviewed changes
Copy link
Contributor

@sandeep-krishnamurthy sandeep-krishnamurthy left a comment

Choose a reason for hiding this comment

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

Thanks for your contributions Vandana. I have 2 comments:

  1. What happens if user pass unsupported dtype?
  2. Please add tests for default case, i.e., I do not pass a dtype fir the Iter

@vandanavk
Copy link
Contributor Author

@sandeep-krishnamurthy I've updated the commit - addressed your review comments. Please have a look at the updated changes and let me know your inputs.

sandeep-krishnamurthy
sandeep-krishnamurthy approved these changes Jul 25, 2018
View reviewed changes
Copy link
Contributor

@sandeep-krishnamurthy sandeep-krishnamurthy left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks.

@Roshrini
Copy link
Member

Thanks for working on this. LGTM.

@vandanavk
Copy link
Contributor Author

Thanks @haojin2 @apeforest @sandeep-krishnamurthy @Roshrini

@szha Can this change be merged?

@sandeep-krishnamurthy sandeep-krishnamurthy merged commit f5b95b0 into apache:master Jul 26, 2018
XinYao1994 pushed a commit to XinYao1994/incubator-mxnet that referenced this pull request Aug 29, 2018
@vandanavk @sandeep-krishnamurthy
Support integer type in ImageIter (apache#11864)
6f15df9
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@haojin2 haojin2 haojin2 left review comments

@apeforest apeforest apeforest requested changes

@sandeep-krishnamurthy sandeep-krishnamurthy sandeep-krishnamurthy approved these changes

@szha szha Awaiting requested review from szha szha is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@vandanavk @Roshrini @sandeep-krishnamurthy @apeforest @haojin2