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

WIP: Fix display of images with non-1 indices #9

Merged
merged 5 commits into from
Apr 12, 2017
Merged

WIP: Fix display of images with non-1 indices #9

merged 5 commits into from
Apr 12, 2017

Conversation

timholy
Copy link
Member

@timholy timholy commented Mar 13, 2017

Until I finish my fixes to restrict, we don't know if there are more changes needed. So don't merge yet.

@codecov-io
Copy link

codecov-io commented Mar 14, 2017

Codecov Report

Merging #9 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master     #9   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           4      4           
  Lines         117    120    +3     
=====================================
+ Hits          117    120    +3
Impacted Files Coverage Δ
src/imshow.jl 100% <100%> (ø) ⬆️
src/encodeimg.jl 100% <100%> (ø) ⬆️

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 9d134d5...7900671. Read the comment docs.

@Evizero
Copy link
Member

Evizero commented Apr 11, 2017

This will need a REQUIRE bump for ImageTransformations 0.2.0 after the merge of JuliaLang/METADATA.jl#8789

(just making a note before I forget about it)

@timholy
Copy link
Member Author

timholy commented Apr 11, 2017

I have a more complete PR coming, just waiting for that METADATA PR to merge. I'm not sure the tests cover enough cases of non-1 indices, though, so I'll be curious to see what you think.

@timholy
Copy link
Member Author

timholy commented Apr 12, 2017

OK, this sort of works. Two issues:

  • are there important cases that need testing but aren't?
  • one oddity is that if you run the tests in the REPL, they pass, but if you run the tests a second time in the same session, they don't. That doesn't seem to happen on master.

@Evizero
Copy link
Member

Evizero commented Apr 12, 2017

I'll take a look after a wake-up coffee. Thanks for doing this!

@Evizero
Copy link
Member

Evizero commented Apr 12, 2017

The test oddity that you ran into was a consequence of the last tests overwriting the default color-depth to 24 bit https://github.com/JuliaImages/ImageInTerminal.jl/blob/master/test/tst_baseshow.jl#L20. Thus running the tests again caused the tests to fail that build on the assumption that the default depth is 256.

I pushed a fix to your branch

@Evizero
Copy link
Member

Evizero commented Apr 12, 2017

Concerning tests. I think the current tests look great. I like how you reuse other reference files. This tests output consistency, and keeps the memory footprint a bit in check.

@Evizero
Copy link
Member

Evizero commented Apr 12, 2017

Test checklist for the changed functions:

  • imshow
  • encodeimg(::SmallBlocks, ::TermColorDepth, ::Matrix) (lighthouse)
  • encodeimg(::BigBlocks, ::TermColorDepth, ::Matrix) (lena)
  • encodeimg(::SmallBlocks, ::TermColorDepth, ::Vector)
  • encodeimg(::BigBlocks, ::TermColorDepth, ::Vector)

To cover the two remaining Vector tests, an adaption of those two testsets should suffice.

@Evizero
Copy link
Member

Evizero commented Apr 12, 2017

A quick Vector test in the REPL revealed the following remaining OffsetArray obstacle in ImageTransformations: https://github.com/JuliaImages/ImageTransformations.jl/blob/master/src/resizing.jl#L180

@Evizero
Copy link
Member

Evizero commented Apr 12, 2017

If you are too busy for these additional changes in this PR, let me know. I can't implement them right this instance, but I'd be happy to take care of them soonish.

@timholy
Copy link
Member Author

timholy commented Apr 12, 2017

An error I first saw yesterday with ImageCore cropped up on AppVeyor. I don't know why that happened, but hopefully the fix is easy.

REQUIRE Outdated
@@ -2,5 +2,5 @@ julia 0.5
Crayons 0.1.0
ColorTypes 0.3.2
ImageCore 0.1.1
ImageTransformations 0.1.0
ImageTransformations 0.2.0
Copy link
Member

Choose a reason for hiding this comment

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

needs a bump to 0.2.1

@Evizero
Copy link
Member

Evizero commented Apr 12, 2017

Very cool. I am excited about this change!

@Evizero
Copy link
Member

Evizero commented Apr 12, 2017

lgtm. go for merge after CI passes?

@Evizero Evizero merged commit f83d17d into master Apr 12, 2017
@Evizero Evizero deleted the teh/non1 branch April 12, 2017 21:19
@Evizero
Copy link
Member

Evizero commented Apr 12, 2017

Thanks! As primarily a terminal user this change will make hacking on the WarpedView a lot nicer

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.

3 participants