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] Switch order of reduce_zero_label and applying label_map in 1.x #2517

Merged
merged 3 commits into from
Jan 30, 2023
Merged

[Fix] Switch order of reduce_zero_label and applying label_map in 1.x #2517

merged 3 commits into from
Jan 30, 2023

Conversation

siddancha
Copy link
Contributor

This is an almost exact duplicate of #2500 (that was made to the master branch) now applied to the 1.x branch.


Motivation

I want to fix a bug through this PR. The bug occurs when two options -- reduce_zero_label=True, and custom classes are used. reduce_zero_label remaps the GT seg labels by remapping the zero-class to 255 which is ignored. Conceptually, this should occur before the label_map is applied, which maps already reduced labels. However, currently, the label_map is applied before the zero label is reduced.

Modification

The modification is simple:

  • I've just interchanged the order of the two operations by moving a few lines from bottom to top.
  • I've added a test that passes when the fix is introduced, and fails on the original master branch.

BC-breaking (Optional)

I do not anticipate this change braking any backward-compatibility.

Checklist

  • Pre-commit or other linting tools are used to fix the potential lint issues.
    • I've fixed all linting/pre-commit errors.
  • The modification is covered by complete unit tests. If not, please add more unit test to ensure the correctness.
    • I've added a unit test.
  • If the modification has potential influence on downstream projects, this PR should be tested with downstream projects, like MMDet or MMDet3D.
    • I don't think this change affects MMDet or MMDet3D.
  • The documentation has been modified accordingly, like docstring or example tutorials.
    • This change fixes an existing bug and doesn't require modifying any documentation/docstring.

@siddancha siddancha changed the title [Fix] Switch order of reduce_zero_label and applying label_map [Fix] Switch order of reduce_zero_label and applying label_map in 1.x Jan 29, 2023
@MeowZheng MeowZheng self-requested a review January 29, 2023 02:56
Copy link
Collaborator

@MeowZheng MeowZheng left a comment

Choose a reason for hiding this comment

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

we are still working on fixing ci. :(

@MeowZheng
Copy link
Collaborator

MeowZheng commented Jan 30, 2023

we have fixed ci problem in #2519 and please rebase this modification on it

@siddancha
Copy link
Contributor Author

Great! Merged the latest version of dev-1.x.

@codecov
Copy link

codecov bot commented Jan 30, 2023

Codecov Report

Base: 83.39% // Head: 83.40% // Increases project coverage by +0.01% 🎉

Coverage data is based on head (ad43e68) compared to base (b9b5d8b).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files
@@             Coverage Diff             @@
##           dev-1.x    #2517      +/-   ##
===========================================
+ Coverage    83.39%   83.40%   +0.01%     
===========================================
  Files          145      145              
  Lines         8510     8510              
  Branches      1274     1274              
===========================================
+ Hits          7097     7098       +1     
+ Misses        1198     1197       -1     
  Partials       215      215              
Flag Coverage Δ
unittests 83.40% <100.00%> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
mmseg/datasets/transforms/loading.py 85.60% <100.00%> (ø)
mmseg/datasets/transforms/transforms.py 90.53% <0.00%> (+0.12%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@MeowZheng MeowZheng mentioned this pull request Jan 30, 2023
@MeowZheng MeowZheng merged commit 74e8b89 into open-mmlab:dev-1.x Jan 30, 2023
@siddancha siddancha deleted the sid/pr/fix-label-map-reduce-zero-order-1.x branch January 30, 2023 04:53
aravind-h-v pushed a commit to aravind-h-v/mmsegmentation that referenced this pull request Mar 27, 2023
* Sequential cpu offload: require accelerate 0.14.0.

* Import is_accelerate_version

* Missing copy.
nahidnazifi87 pushed a commit to nahidnazifi87/mmsegmentation_playground that referenced this pull request Apr 5, 2024
… 1.x (open-mmlab#2517)

This is an almost exact duplicate of open-mmlab#2500 (that was made to the
`master` branch) now applied to the `1.x` branch.

---

## Motivation

I want to fix a bug through this PR. The bug occurs when two options --
`reduce_zero_label=True`, and custom classes are used.
`reduce_zero_label` remaps the GT seg labels by remapping the zero-class
to 255 which is ignored. Conceptually, this should occur *before* the
`label_map` is applied, which maps *already reduced labels*. However,
currently, the `label_map` is applied before the zero label is reduced.

## Modification

The modification is simple:
- I've just interchanged the order of the two operations by moving a few
lines from bottom to top.
- I've added a test that passes when the fix is introduced, and fails on
the original `master` branch.

## BC-breaking (Optional)

I do not anticipate this change braking any backward-compatibility.

## Checklist

- [x] Pre-commit or other linting tools are used to fix the potential
lint issues.
  - _I've fixed all linting/pre-commit errors._
- [x] The modification is covered by complete unit tests. If not, please
add more unit test to ensure the correctness.
  - _I've added a unit test._ 
- [x] If the modification has potential influence on downstream
projects, this PR should be tested with downstream projects, like MMDet
or MMDet3D.
  - _I don't think this change affects MMDet or MMDet3D._
- [x] The documentation has been modified accordingly, like docstring or
example tutorials.
- _This change fixes an existing bug and doesn't require modifying any
documentation/docstring._
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.

None yet

4 participants