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

Refine the documentation of im2rec #12606

Merged
sandeep-krishnamurthy merged 5 commits into from
Sep 20, 2018
Merged

Refine the documentation of im2rec #12606

sandeep-krishnamurthy merged 5 commits into from
Sep 20, 2018

Conversation

@stu1130
Copy link
Contributor

@stu1130 stu1130 commented Sep 19, 2018

Description

Currently, we have two im2rec tools. One is python, the other one is C++. There are slightly different in terms of functionality. It helps to solve the #11884 as well.

  • change the data format on example
  • add some notes to tell users the difference and add corresponding to each other.

Checklist

Essentials

Please feel free to remove inapplicable items for your PR.

  • The PR title starts with [MXNET-$JIRA_ID], where $JIRA_ID refers to the relevant JIRA issue created (except PRs with tiny changes)
  • 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)
  • Nightly tests are added for complicated/long-running ones (e.g. changing distributed kvstore)
  • Build tests will be added for build configuration changes (e.g. adding a new build option with NCCL)
  • Code is well-documented:
  • For user-facing API changes, API doc string has been updated.
  • For new C++ functions in header files, their functionalities and arguments are documented.
  • For new examples, README.md is added to explain the what the example does, the source of the dataset, expected performance on test set and reference to the original paper if applicable
  • Check the API doc at http://mxnet-ci-doc.s3-accelerate.dualstack.amazonaws.com/PR-$PR_ID/$BUILD_ID/index.html
  • To the my best knowledge, examples are either not affected by this change, or have been fixed to be compatible with this change

Changes

Comments

@aaronmarkham
Please let me know how can I make it less confused.

@stu1130 stu1130 requested a review from szha as a code owner September 19, 2018 23:01
@stu1130
Copy link
Contributor Author

stu1130 commented Sep 19, 2018

@mxnet-label-bot[pr-awaiting-review]

@marcoabreu marcoabreu added the pr-awaiting-review PR is waiting for code review label Sep 19, 2018
lupesko
lupesko suggested changes Sep 20, 2018
View reviewed changes
Copy link
Contributor

@lupesko lupesko 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, just one clarification.

docs/faq/recordio.md
972457 633 n07723039_1627.JPEG
7534 11 n01630670_4486.JPEG
1191261 249 n12407079_5106.JPEG
95099 464.000000 n04467665_17283.JPEG
Copy link
Contributor

@lupesko lupesko Sep 20, 2018

Choose a reason for hiding this comment

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

Is the decimal point really required? this is a bit weird.

Copy link
Contributor Author

@stu1130 stu1130 Sep 20, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

If the data.lst file is generated by im2rec.py instead of doing it manually, the label will have those decimal point. I think it would be less confused for users?
And the reason why it uses floating point is that the label value could be generated by the regression, e.g. 68.6 kg for a human body weight.

sandeep-krishnamurthy
sandeep-krishnamurthy approved these changes Sep 20, 2018
View reviewed changes
Copy link

@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.
However, we need to revisit why do we need 2 versions and if required, why is there a discrepency in functionality.

@sandeep-krishnamurthy sandeep-krishnamurthy merged commit d4adbd2 into apache:master Sep 20, 2018
aaronmarkham
aaronmarkham reviewed Sep 20, 2018
View reviewed changes
Copy link
Contributor

@aaronmarkham aaronmarkham left a comment

Choose a reason for hiding this comment

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

I realize this was already merged, but I have suggestions nonetheless.

docs/faq/recordio.md
* RecordIO has a simple way to partition, simplifying distributed setting. We provide an example later.

We provide the [im2rec tool](https://github.com/dmlc/mxnet/blob/master/tools/im2rec.cc) so you can create an Image RecordIO dataset by yourself. The following walkthrough shows you how.
We provide the [im2rec tool](https://github.com/dmlc/mxnet/blob/master/tools/im2rec.cc) so you can create an Image RecordIO dataset by yourself. The following walkthrough shows you how. Note that there is python version of [im2rec tool](https://github.com/apache/incubator-mxnet/blob/master/tools/im2rec.py) and [example](https://mxnet.incubator.apache.org/tutorials/basic/data.html) using real-world data.
Copy link
Contributor

@aaronmarkham aaronmarkham Sep 20, 2018

Choose a reason for hiding this comment

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

We provide two tools for creating a RecordIO dataset.

  • im2rec.cc - implements the tool using the C++ API.
  • im2rec.py - implements the tool using the Python API.

Both provide the same output: a RecordIO dataset.
(Then take this mention and add it later for "Next Steps". I don't think you want them leaving this FAQ/tutorial quite yet.)
You may want to also review the example using real-world data with im2rec.py.

docs/faq/recordio.md

### Step 1. Make an Image List File

* Note that the im2rec.py provide a param `--list` to generate the list for you but im2rec.cc doesn't support it.
Copy link
Contributor

@aaronmarkham aaronmarkham Sep 20, 2018

Choose a reason for hiding this comment

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

provides

Copy link
Contributor

@aaronmarkham aaronmarkham Sep 20, 2018

Choose a reason for hiding this comment

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

you, but

docs/tutorials/basic/data.md
```

#### Using tools/im2rec.py
You can also convert raw images into *RecordIO* format using the ``im2rec.py`` utility script that is provided in the MXNet [src/tools](https://github.com/dmlc/mxnet/tree/master/tools) folder.
Copy link
Contributor

@aaronmarkham aaronmarkham Sep 20, 2018

Choose a reason for hiding this comment

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

Update the repo link

docs/tutorials/basic/data.md
You can also convert raw images into *RecordIO* format using the ``im2rec.py`` utility script that is provided in the MXNet [src/tools](https://github.com/dmlc/mxnet/tree/master/tools) folder.
An example of how to use the script for converting to *RecordIO* format is shown in the `Image IO` section below.

* Note that there is a C++ version of [im2rec](https://github.com/dmlc/mxnet/blob/master/tools/im2rec.cc), please refer to [here](https://mxnet.incubator.apache.org/faq/recordio.html) for more information.
Copy link
Contributor

@aaronmarkham aaronmarkham Sep 20, 2018

Choose a reason for hiding this comment

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

Please don't link only "here". Provide the full description of what the link is going to.
Note that there is a C++ API implementation of im2rec. Refer to the RecordIO FAQ for more information.

@stu1130 stu1130 mentioned this pull request Sep 20, 2018
Merged
5 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

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

3 more reviewers

@aaronmarkham aaronmarkham aaronmarkham left review comments

@lupesko lupesko lupesko requested changes

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

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

pr-awaiting-review PR is waiting for code review

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@stu1130 @lupesko @sandeep-krishnamurthy @aaronmarkham @marcoabreu