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

switch band order to scarlet band order #113

Merged
merged 4 commits into from
Mar 12, 2021
Merged

Conversation

ismael-mendoza
Copy link
Collaborator

@ismael-mendoza ismael-mendoza commented Mar 11, 2021

This is more of a selfish change, I've always wanted the order of the images shape to be [n_bands, nx, ny] rather than [nx, ny, n_bands]. This is also the convention used in scarlet and in bliss so it would be nice to have everything consistent and not have to remember to transpose (at least for me). This also simplifies the code in plot_utils.py slightly.

Would this cause any problems for anyone? Did I miss any places?

@codecov
Copy link

codecov bot commented Mar 11, 2021

Codecov Report

Merging #113 (0000a1a) into main (20d1631) will increase coverage by 0.05%.
The diff coverage is 92.85%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #113      +/-   ##
==========================================
+ Coverage   74.57%   74.63%   +0.05%     
==========================================
  Files          10       10              
  Lines         948      954       +6     
==========================================
+ Hits          707      712       +5     
- Misses        241      242       +1     
Flag Coverage Δ
unittests 74.63% <92.85%> (+0.05%) ⬆️

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

Impacted Files Coverage Δ
btk/draw_blends.py 90.90% <88.88%> (-0.26%) ⬇️
btk/measure.py 84.74% <100.00%> (ø)
btk/plot_utils.py 46.15% <100.00%> (ø)

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 20d1631...0000a1a. Read the comment docs.

@thuiop
Copy link
Collaborator

thuiop commented Mar 11, 2021

Well I do usually have the channels as the last dimension... maybe we should have a tag defining the order and doing the transposition automatically ?

@ismael-mendoza
Copy link
Collaborator Author

Well I do usually have the channels as the last dimension... maybe we should have a tag defining the order and doing the transposition automatically ?

Yeah I can do that, I think I will use this order as the default for everything, but add an optional keyword argument to the draw_blend_generator so that it can return the images transposed in a specific way. Would that work for you?

@ismael-mendoza
Copy link
Collaborator Author

OK I just added a keyword dim_order, let me know what you think @thuiop

@thuiop
Copy link
Collaborator

thuiop commented Mar 11, 2021

I think it works that way. Tensorflow uses a string with values "NHWC" or "NCHW" as a tag (for channel last and channel first), this may be more user-friendly ? I don't think anyone would want to have the channels as the middle one anyway. But it's a very minor thing anyway.

thuiop
thuiop previously approved these changes Mar 11, 2021
Copy link
Collaborator

@thuiop thuiop left a comment

Choose a reason for hiding this comment

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

See my last comment ; apart from that it seems OK for me, as long as the tests run it should be working.

@ismael-mendoza
Copy link
Collaborator Author

I think it works that way. Tensorflow uses a string with values "NHWC" or "NCHW" as a tag (for channel last and channel first), this may be more user-friendly ? I don't think anyone would want to have the channels as the middle one anyway. But it's a very minor thing anyway.

Ah I like this idea better actually, I will quickly make the change and then merge when tests are passing. Thanks!

@ismael-mendoza ismael-mendoza merged commit e34eb99 into main Mar 12, 2021
@ismael-mendoza ismael-mendoza deleted the im/switch_band_order branch March 12, 2021 14:25
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.

2 participants