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.

Sign up for GitHub

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

Jump to bottom

Add tiling documentation #2204

Closed
wants to merge 25 commits into from
Closed

Add tiling documentation #2204

wants to merge 25 commits into from

Conversation

eugene123tw
Copy link
Contributor

Summary

How to test

Checklist

  • I have added unit tests to cover my changes.​
  • I have added integration tests to cover my changes.​
  • I have added e2e tests for validation.
  • I have added the description of my changes into CHANGELOG in my target branch (e.g., CHANGELOG in develop).​
  • I have updated the documentation in my target branch accordingly (e.g., documentation in develop).
  • I have linked related issues.

License

  • I submit my code changes under the same Apache License that covers the project.
    Feel free to contact the maintainers if that's a concern.
  • I have updated the license header for each file (see an example below).
# Copyright (C) 2023 Intel Corporation
# SPDX-License-Identifier: Apache-2.0

@github-actions github-actions bot added the DOC Improvements or additions to documentation label May 30, 2023
@eugene123tw eugene123tw changed the title add tiling doc Add tiling documentation May 30, 2023
@eugene123tw eugene123tw marked this pull request as ready for review May 30, 2023 14:20
@eugene123tw eugene123tw requested a review from a team as a code owner May 30, 2023 14:20
@eugene123tw eugene123tw assigned jaegukhyun and unassigned jaegukhyun May 30, 2023
@eugene123tw eugene123tw requested a review from jaegukhyun May 30, 2023 14:21
GalyaZalesskaya
GalyaZalesskaya reviewed May 31, 2023
View reviewed changes
Copy link
Contributor

@GalyaZalesskaya GalyaZalesskaya left a comment

Choose a reason for hiding this comment

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

Thank you for your work, Eugene!

Can we consider moving everything including Enable Tiling via OTX Training CLI and further into a separate page in the tutorial section, since the code snippets look more natural there?
The Explanation section usually elaborates on the used algorithms.
@kprokofi I'd like to hear your opinion as well

docs/source/guide/explanation/additional_features/tiling.rst Outdated Show resolved Hide resolved
docs/source/guide/explanation/additional_features/tiling.rst Show resolved Hide resolved
docs/source/guide/explanation/additional_features/tiling.rst Outdated Show resolved Hide resolved
docs/source/guide/explanation/additional_features/tiling.rst Outdated Show resolved Hide resolved
docs/source/guide/explanation/additional_features/tiling.rst Show resolved Hide resolved
docs/source/guide/explanation/additional_features/tiling.rst Show resolved Hide resolved
sungmanc
sungmanc previously approved these changes Jun 1, 2023
View reviewed changes
Copy link
Contributor

@sungmanc sungmanc left a comment

Choose a reason for hiding this comment

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

From my side, I don't have comments on this documentation. Thanks for the hard work

docs/source/guide/explanation/additional_features/tiling.rst Outdated Show resolved Hide resolved
docs/source/guide/explanation/additional_features/tiling.rst Outdated Show resolved Hide resolved
@eugene123tw eugene123tw requested review from kprokofi and sungmanc June 12, 2023 11:05
@eugene123tw
Copy link
Contributor Author

Thank you for your work, Eugene!

Can we consider moving everything including Enable Tiling via OTX Training CLI and further into a separate page in the tutorial section, since the code snippets look more natural there? The Explanation section usually elaborates on the used algorithms. @kprokofi I'd like to hear your opinion as well

Can you please add another option, where we build a workplace and then manually update the template.yaml to use tiling and update tiling parameters?

`otx build Custom_Object_Detection_Gen3_SSD --train-data-roots tests/assets/small_objects --val-data-roots tests/assets/small_objects params

cd workspace-DETECTION

update template
train
`

@GalyaZalesskaya, regarding manually tuning parameters in template.yaml. Do we have a separate documentation for that so I could add a note and guide users there?

sungmanc
sungmanc previously approved these changes Jun 12, 2023
View reviewed changes
GalyaZalesskaya
GalyaZalesskaya reviewed Jun 15, 2023
View reviewed changes
Copy link
Contributor

@GalyaZalesskaya GalyaZalesskaya left a comment

Choose a reason for hiding this comment

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

Thank you so much for even more clarification, great work! I have only a few comments left related to code examples.

Also, I thought about your question there to put the code examples including template.yaml update. But looks like it's better to keep it here

docs/source/guide/explanation/additional_features/tiling.rst Outdated Show resolved Hide resolved
docs/source/guide/explanation/additional_features/tiling.rst Show resolved Hide resolved
docs/source/guide/explanation/additional_features/tiling.rst Show resolved Hide resolved
docs/source/guide/explanation/additional_features/tiling.rst Outdated Show resolved Hide resolved
@eugene123tw eugene123tw requested a review from jaegukhyun July 10, 2023 09:58
GalyaZalesskaya
GalyaZalesskaya previously approved these changes Jul 10, 2023
View reviewed changes
Copy link
Contributor

@GalyaZalesskaya GalyaZalesskaya left a comment

Choose a reason for hiding this comment

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

<3

goodsong81
goodsong81 previously approved these changes Jul 10, 2023
View reviewed changes
Copy link
Contributor

@goodsong81 goodsong81 left a comment

Choose a reason for hiding this comment

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

Thank you for the detailed description. LGTM!

@eugene123tw eugene123tw dismissed stale reviews from goodsong81 and GalyaZalesskaya via 9fd8a05 July 10, 2023 10:22
goodsong81
goodsong81 previously approved these changes Jul 10, 2023
View reviewed changes
@sungmanc
Copy link
Contributor

How about changing the target branch?

jaegukhyun and others added 3 commits July 10, 2023 20:25
@jaegukhyun
Fix detection e2e tests (openvinotoolkit#2327)
19477a3 
Fix for detection
@sungmanc @goodsong81
Mergeback: Label addtion/deletion 1.2.4 --> 1.4.0 (openvinotoolkit#2326)
6b51ba0 
* Make black happy

* Fix conflicts

* Merge-back: add test datasets and edit the test code

* Make black happy

* Fix mis-merge

* Make balck happy

* Fix typo

* Fix typoi

---------

Co-authored-by: Songki Choi <songki.choi@intel.com>
@yunchu
Bump datumaro up to 1.4.0rc2 (openvinotoolkit#2332)
b47ebf3 
bump datumaro up to 1.4.0rc2
@eugene123tw eugene123tw changed the base branch from develop to releases/1.4.0 July 11, 2023 08:10
@eugene123tw eugene123tw dismissed goodsong81’s stale review July 11, 2023 08:10

The base branch was changed.

@eugene123tw eugene123tw changed the base branch from releases/1.4.0 to develop July 11, 2023 08:10
@github-actions github-actions bot added DEPENDENCY Any changes in any dependencies (new dep or its version) should be produced via Change Request on PM API Any changes in OTX API ALGO Any changes in OTX Algo Tasks implementation TEST Any changes in tests labels Jul 11, 2023
@eugene123tw eugene123tw changed the base branch from develop to releases/1.4.0 July 11, 2023 08:25
@github-actions github-actions bot added CLI Any changes in OTE CLI and removed DEPENDENCY Any changes in any dependencies (new dep or its version) should be produced via Change Request on PM API Any changes in OTX API labels Jul 11, 2023
@eugene123tw eugene123tw changed the base branch from releases/1.4.0 to develop July 11, 2023 08:26
@eugene123tw
Copy link
Contributor Author

git commits are messed up, so I created two PRs. Sorry for the bother

@eugene123tw eugene123tw deleted the eugene/tiling-doc branch July 11, 2023 08:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@goodsong81 goodsong81 goodsong81 left review comments

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

@sungmanc sungmanc Awaiting requested review from sungmanc

@jaegukhyun jaegukhyun Awaiting requested review from jaegukhyun

@GalyaZalesskaya GalyaZalesskaya Awaiting requested review from GalyaZalesskaya

Assignees
No one assigned
Labels
ALGO Any changes in OTX Algo Tasks implementation CLI Any changes in OTE CLI DOC Improvements or additions to documentation TEST Any changes in tests
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.