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 usage of channel_order in loading.py #271

Merged
merged 3 commits into from
Apr 24, 2021

Conversation

ckkelvinchan
Copy link
Member

In the previous version, LoadImageFromFileList and LoadPairedImageFromFile did not use channel_order in mmcv.imfrombytes. Therefore, the input is still in bgr order even if channel_order='rgb is specified in the configuration file.

This PR adds the argument to the function. The default value is bgr, and hence it does not affect the existing codes.

@ckkelvinchan
Copy link
Member Author

@innerlee Please note that RandomLoadResizeBg also did not use channel_order, but I am not sure whether this is necessary. Therefore I leave it unchanged.

@codecov
Copy link

codecov bot commented Apr 23, 2021

Codecov Report

Merging #271 (eebdfc7) into master (8d21735) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #271      +/-   ##
==========================================
- Coverage   79.95%   79.94%   -0.02%     
==========================================
  Files         159      159              
  Lines        7939     7942       +3     
  Branches     1176     1177       +1     
==========================================
+ Hits         6348     6349       +1     
- Misses       1447     1449       +2     
  Partials      144      144              
Flag Coverage Δ
unittests 79.94% <100.00%> (-0.02%) ⬇️

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

Impacted Files Coverage Δ
mmedit/datasets/pipelines/loading.py 95.23% <100.00%> (+0.02%) ⬆️
mmedit/apis/test.py 25.25% <0.00%> (-0.53%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8d21735...eebdfc7. Read the comment docs.

@innerlee
Copy link
Contributor

innerlee commented Apr 23, 2021

RandomLoadResizeBg is safe, since it can pass any kwargs (including channel_order of course) to the file client.

edit: sorry no. Its imfrombytes. Yes please add the option to RandomLoadResizeBg, it is safe since there is no config actually uses it yet.

@innerlee
Copy link
Contributor

innerlee commented Apr 23, 2021

@Yshuo-Li This bug will affect the checkpoint of LIIF, because the channel_order in the config is not passed to image decoder. So the model accepts BGR instead of RGB

@ckkelvinchan
Copy link
Member Author

RandomLoadResizeBg is safe, since it can pass any kwargs (including channel_order of course) to the file client.

edit: sorry no. Its imfrombytes. Yes please add the option to RandomLoadResizeBg, it is safe since there is no config actually uses it yet.

Okay, will do

@Yshuo-Li
Copy link
Collaborator

@Yshuo-Li This bug will affect the checkpoint of LIIF, because the channel_order in the config is not passed to image decoder. So the model accepts BGR instead of RGB

LIIF uses LoadImageFromFile instead of LoadImageFromFileList
In LoadImageFromFile line 73:
img = mmcv.imfrombytes( img_bytes, flag=self.flag, channel_order=self.channel_order)

@innerlee
Copy link
Contributor

@ckkelvinchan since this change will affect the checkpoint, do you want to re-train one?

@ckkelvinchan
Copy link
Member Author

@ckkelvinchan since this change will affect the checkpoint, do you want to re-train one?

I am reproducing GLEAN, so I may need to do it later. I suggest changing the configuration files for BasicVSR and IconVSR back to bgr for the moment.

@innerlee innerlee merged commit ce4ad1f into open-mmlab:master Apr 24, 2021
@wwhio
Copy link
Contributor

wwhio commented Apr 27, 2021

I suggest changing the configuration files for BasicVSR and IconVSR back to bgr for the moment.

Hello @ckkelvinchan , thank you for your awesome work. Will this change affect the optical flow estimated by SPyNet ? According to this code, SPyNet takes an image with bgr channel order.

@ckkelvinchan
Copy link
Member Author

ckkelvinchan commented Apr 27, 2021

The SPyNet we used takes RGB as inputs. This is slightly different from the one you mentioned. That's why we did not change it to bgr. We should keep rgb.

@wwhio
Copy link
Contributor

wwhio commented Apr 27, 2021

OK. Thank you.

Yshuo-Li pushed a commit to Yshuo-Li/mmediting that referenced this pull request Jul 15, 2022
* Fix usage of channel_order in loading.py

* Add channel_order to RandomLoadResizeBg

* Update weights for rgb order
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