Skip to content

Conversation

@avdmitry
Copy link
Contributor

Module for loading datasets for Fall2014 OpenCV Challenge.

I hightly unsure about files structure, because I worked on separate application and just converted it to module today (we need hurry as Challenge should start September, 1).

Document in doc/ is very first version and will be updated.

src/ & include/ contain main code.
samples/ - samples how to work with this code.

tinyxml2/ - 3rd party library, in my project structure was in 3rdparty/ folder, but this didn't work after converting to module.

I only checked that module was successfully built.

@avdmitry
Copy link
Contributor Author

@vpisarev, please, take a look on the code, as September, 1 getting closer :)
Looks like samples are not built after I converted project to the module..

@bmagyar
Copy link
Contributor

bmagyar commented Aug 28, 2014

Hi @avdmitry,

please take a look at the buildbot status for your PR.
The basic rule of thumb is that until all is green, your PR won't be merged. You can keep making commits to your PR branch, the buildbot will automatically rebuild everything and give you new results.

One thing that I'm pretty sure is not allowed, is to commit the code of tinyxml2 into the repo directly. The tinyxml dependency might be an issue altogether.

For more info about contribution in general look on the OpenCV contribution guide, OpenCV contribution process and OpenCV coding guide. If you comply with all these, that will make the review process easier and more rewarding for both parties :)

@avdmitry
Copy link
Contributor Author

Hello, @bmagyar

thank you for explanation. I read documents, but however it is necessary to practice a little.
One problem here that deadline is really close)

@vpisarev vpisarev self-assigned this Aug 29, 2014
@vpisarev
Copy link
Contributor

@avdmitry, @bmagyar, dependency of tinyxml (or tinyxml2) is probably fine, since 1) it's rather compact 2) dmitry put it right to the module, so there is no extra dependency, 3) the license permits us to do so.

as Bence mentioned, the builders should be fixed, they all should be green. Also, putting generated HTML docs is inappropriate. Just put rst files. I will review the pull request more carefully soon, but please correct the build problems first

@avdmitry
Copy link
Contributor Author

@vpisarev, finally, "Required builds passed")

Copy link
Contributor

Choose a reason for hiding this comment

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

all the headers in this module should be put into datasettools subdirectory

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.

@avdmitry
Copy link
Contributor Author

avdmitry commented Sep 2, 2014

@vpisarev, last commit fixed all comments except base class enhancment and interface-implementation pattern.

@avdmitry
Copy link
Contributor Author

avdmitry commented Sep 2, 2014

enhanced base class: created hierarchy for objects in train\test and moved train\test to base class (several unused lines remain in dataset.hpp & fr_lfw.hpp - I'll clean them in next commit)

@vpisarev
Copy link
Contributor

vpisarev commented Sep 2, 2014

@avdmitry, yes, this is much better. Since you are going to make another commit, as you said, can you also capitalize all the class names?

@avdmitry
Copy link
Contributor Author

avdmitry commented Sep 2, 2014

@vpisarev, I have already fixed comment that class names should start from capital letter, probably all structures also should start from capital letter, I'll fix them too.

@avdmitry
Copy link
Contributor Author

avdmitry commented Sep 3, 2014

@vpisarev, @Nerei all comments were fixed, except interface-implementation pattern, WIP

vpisarev added a commit that referenced this pull request Sep 3, 2014
@vpisarev vpisarev merged commit 89c0435 into opencv:master Sep 3, 2014
@avdmitry avdmitry deleted the datasetstools branch September 21, 2014 10:51
allnes pushed a commit to allnes/opencv_contrib that referenced this pull request Jun 9, 2024
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.

3 participants