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

fix #2968 #2986

Merged
merged 2 commits into from
Feb 12, 2024
Merged

fix #2968 #2986

merged 2 commits into from
Feb 12, 2024

Conversation

SidneyLann
Copy link
Contributor

fix #2968

@SidneyLann SidneyLann requested review from zachgk, frankfliu and a team as code owners February 10, 2024 12:58
@@ -75,7 +75,7 @@ protected DetectedObjects processFromBoxOutput(NDList list) {
float classProb = buf[index + c];
if (classProb > maxClassProb) {
maxClassProb = classProb;
maxIndex = c;
maxIndex = c - 4;
Copy link
Contributor

Choose a reason for hiding this comment

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

maxIndex = c is corret.

nClasses should be the same as classes.length. We might add a check to ensure we provide correct error message.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No. maxIndex should start with 0, but not 4!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The labels 0, 1, 2, 3 should never be detected for the bug.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Or, for (int c = 4; c < nClasses; c++) should change to for (int c = 0; c < nClasses; c++)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

maybe this is correct:
for (int c = 0; c < nClasses; c++) {
float classProb = buf[index + 4 + c];
if (classProb > maxClassProb) {
maxClassProb = classProb;
maxIndex = c;
}
}

Copy link
Contributor

Choose a reason for hiding this comment

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

I see where is the problem. Original COCO dataset has 80 classes, but in synset.txt contains 84 rows and first 4 are dummy:

0 = "# Classes for coco dataset on which yelov8 is trained"
1 = "# source config https://github.com/ultralytics/ultralytics/blob/main/ultralytics/cfg/datasets/coco.yaml."
2 = "# COCO dataset website: https://cocodataset.org/#home"
3 = "# Ultralytics Coco doc page: https://docs.ultralytics.com/datasets/detect/coco/"

So if you use YoloV8Translator, you need pad 4 dummy labels.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if (padding != 0 && padding != 4) { //means padding ==0 or padding ==4 is invalid, this is not true. the check should be removed and change for (int c = 4; c < nClasses; c++) { to for (int c = padding; c < nClasses; c++) {?

@codecov-commenter
Copy link

codecov-commenter commented Feb 12, 2024

Codecov Report

Attention: 1341 lines in your changes are missing coverage. Please review.

Comparison is base (bb5073f) 72.08% compared to head (6ed703e) 72.28%.
Report is 992 commits behind head on master.

Files Patch % Lines
...va/ai/djl/modality/nlp/generate/TextGenerator.java 2.81% 276 Missing ⚠️
.../java/ai/djl/modality/nlp/generate/SeqBatcher.java 0.75% 132 Missing ⚠️
...ity/nlp/generate/ContrastiveSeqBatchScheduler.java 2.97% 98 Missing ⚠️
...i/djl/modality/nlp/generate/SeqBatchScheduler.java 9.83% 55 Missing ⚠️
.../java/ai/djl/modality/cv/BufferedImageFactory.java 40.96% 47 Missing and 2 partials ⚠️
...a/ai/djl/modality/nlp/generate/StepGeneration.java 2.04% 48 Missing ⚠️
api/src/main/java/ai/djl/ndarray/NDArray.java 43.42% 39 Missing and 4 partials ⚠️
...n/java/ai/djl/modality/cv/output/CategoryMask.java 22.00% 39 Missing ⚠️
...i/src/main/java/ai/djl/ndarray/NDArrayAdapter.java 71.21% 31 Missing and 7 partials ⚠️
.../cv/translator/SemanticSegmentationTranslator.java 37.50% 35 Missing ⚠️
... and 74 more

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@             Coverage Diff              @@
##             master    #2986      +/-   ##
============================================
+ Coverage     72.08%   72.28%   +0.19%     
- Complexity     5126     7290    +2164     
============================================
  Files           473      722     +249     
  Lines         21970    32525   +10555     
  Branches       2351     3395    +1044     
============================================
+ Hits          15838    23511    +7673     
- Misses         4925     7391    +2466     
- Partials       1207     1623     +416     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@lanking520 lanking520 merged commit 0c9eff7 into deepjavalibrary:master Feb 12, 2024
5 checks passed
@SidneyLann
Copy link
Contributor Author

SidneyLann commented Feb 13, 2024

The merging logic is not correct, it does not allow padding == 4. I think 0 <= padding < nClasses is valid.

@frankfliu
Copy link
Contributor

The output shape is (8400, 84) for COCO dataset, we should always ignore first 4 entries in the output.
Ideally, we should not padding classes with dummy classes, the originally implementation is a hack. Keep the 4 padding behavior is purely for backward compatibility. We should not allow other paddings.

@SidneyLann
Copy link
Contributor Author

But 0 padding or 4 padding is not work now

@frankfliu
Copy link
Contributor

0 or 4 should work, it throw exception when not 0 and not 4

if (padding != 0 && padding != 4) {

@SidneyLann
Copy link
Contributor Author

If padding is 0, the min label is 4, it should be 0

@frankfliu
Copy link
Contributor

I think the variable name padding is misleading. It should diff,

if diff == 0, it mean the classes has already padded with dummy entry, so the index should as is.
If diff == 4, it means the classes doesn't have padding, so we should -4 to adjust the index

@SidneyLann
Copy link
Contributor Author

I finetune yolov8 in my dataset and get a new yolov8 model for 96 classes, there are 96 entries in my synset.txt and has no dummy entries. now the diff is 4(doesn't have padding)?

int padding = nClasses - classes.size();// nClasses is from synset.txt and equals to 96? classes.size() is from the model and equals to 96?

So the padding(diff) is 0?

@frankfliu
Copy link
Contributor

If you trained your model with 96 classes, the output should should be (8400, 100)

nClasses - classes.size() should be 4

frankfliu added a commit that referenced this pull request Apr 26, 2024
* fixes YoloV8Translator bug

* [api] Improves YoloV8Translator dummy classes handling

---------

Co-authored-by: Administrator <Administrator@tech8>
Co-authored-by: Frank Liu <[email protected]>
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.

Hundreds wrong detection on yolov8
4 participants