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

Change docstring location and learning hyperparams for object detection #2699

Merged
merged 1 commit into from
Dec 7, 2023

Conversation

jaegukhyun
Copy link
Contributor

Summary

This PR is for changing learning hyperparameters of object detection recipes.
Furthermore this PR also changes some wrong docstring location

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

@jaegukhyun jaegukhyun marked this pull request as ready for review December 6, 2023 03:48
Copy link
Contributor

@harimkang harimkang left a comment

Choose a reason for hiding this comment

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

I think we need to select one of options
[option1]

# Copyright (C) 2023 Intel Corporation
# SPDX-License-Identifier: Apache-2.0
"""~~~~~"""

[option2]

# Copyright (C) 2023 Intel Corporation
# SPDX-License-Identifier: Apache-2.0

"""~~~~~"""

[option3]

# Copyright (C) 2023 Intel Corporation
# SPDX-License-Identifier: Apache-2.0
#
"""~~~~~"""

This may not seem like a big deal, but it's something that needs to be taken into account to make our code look neat.
Of course, it doesn't have to be resolved in this PR. I just want to hear your thoughts.

@samet-akcay
Copy link
Contributor

I think we need to select one of options [option1]

# Copyright (C) 2023 Intel Corporation
# SPDX-License-Identifier: Apache-2.0
"""~~~~~"""

[option2]

# Copyright (C) 2023 Intel Corporation
# SPDX-License-Identifier: Apache-2.0

"""~~~~~"""

[option3]

# Copyright (C) 2023 Intel Corporation
# SPDX-License-Identifier: Apache-2.0
#
"""~~~~~"""

This may not seem like a big deal, but it's something that needs to be taken into account to make our code look neat. Of course, it doesn't have to be resolved in this PR. I just want to hear your thoughts.

I agree, we need a PR to fix all the docstring and license headers. We will then need to stick to it not to break consistency.

I would personally prefer the following:

"""Docstring Title Here.

Here is some docstring explanation. 
This part will be used within the documentation.
"""

# Copyright (C) 2023 Intel Corporation
# SPDX-License-Identifier: Apache-2.0

from package1 import module1
import library
...

@jaegukhyun
Copy link
Contributor Author

I think we need to select one of options [option1]

# Copyright (C) 2023 Intel Corporation
# SPDX-License-Identifier: Apache-2.0
"""~~~~~"""

[option2]

# Copyright (C) 2023 Intel Corporation
# SPDX-License-Identifier: Apache-2.0

"""~~~~~"""

[option3]

# Copyright (C) 2023 Intel Corporation
# SPDX-License-Identifier: Apache-2.0
#
"""~~~~~"""

This may not seem like a big deal, but it's something that needs to be taken into account to make our code look neat. Of course, it doesn't have to be resolved in this PR. I just want to hear your thoughts.

I prefer @samet-akcay's option which is docstring before License.
If I have to choose option from your suggestion then I would prefer option 1, and I think current v2 branch follow option 1

@jaegukhyun jaegukhyun merged commit 7b3b23e into openvinotoolkit:v2 Dec 7, 2023
6 checks passed
eugene123tw pushed a commit to eugene123tw/training_extensions that referenced this pull request Dec 8, 2023
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.

5 participants