Skip to content

Latest attempt to get this working on 0.7 #65

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

Merged
merged 18 commits into from
Aug 17, 2018
Merged

Latest attempt to get this working on 0.7 #65

merged 18 commits into from
Aug 17, 2018

Conversation

timholy
Copy link
Member

@timholy timholy commented Jul 13, 2018

This incorporates #41 and #64. As I am rushing to leave town for two weeks, most likely without any internet whatsoever, this is quick.

Most of the tests are working on 0.7. You need a fresh build of Julia and you also want JuliaLang/julia#28100 and JuliaLang/julia#28084. While I am gone I would urge members of the Images community to keep trying to get the JuliaImages packages working on 0.7: porting these has shaken out something like a dozen critical bugs in Base Julia and its stdlibs. Having put so much work into Julia and JuliaImages so far, it would be something of a shame if 1.0 got released and we couldn't really run JuliaImages packages.

Discourse advertised a slack channel on Fridays to help people port packages. Good luck!

timholy and others added 5 commits July 12, 2018 18:07
RalphAS added 3 commits July 19, 2018 23:13
With proposed PRs for TiledIterations this gets through the test suite.
A tricky bounds violation and some inference failures remain.
@ajdunlap ajdunlap mentioned this pull request Jul 22, 2018
@codecov
Copy link

codecov bot commented Aug 15, 2018

Codecov Report

Merging #65 into master will increase coverage by 2.97%.
The diff coverage is 96.89%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #65      +/-   ##
==========================================
+ Coverage   91.51%   94.49%   +2.97%     
==========================================
  Files           8        8              
  Lines        1096      690     -406     
==========================================
- Hits         1003      652     -351     
+ Misses         93       38      -55
Impacted Files Coverage Δ
src/ImageFiltering.jl 100% <ø> (ø) ⬆️
src/kernelfactors.jl 97.14% <100%> (+5.87%) ⬆️
src/border.jl 99.01% <100%> (+2.12%) ⬆️
src/utils.jl 100% <100%> (+29.03%) ⬆️
src/specialty.jl 100% <100%> (+10.52%) ⬆️
src/kernel.jl 98.55% <100%> (-1.45%) ⬇️
src/mapwindow.jl 97.87% <100%> (+6.66%) ⬆️
src/imfilter.jl 90.03% <93.22%> (+0.71%) ⬆️
... and 5 more

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 8cf536b...87aa133. Read the comment docs.

@RalphAS
Copy link
Contributor

RalphAS commented Aug 16, 2018

@timholy I wasn't sure whether you meant to just merge into this branch or fully into master.

Earlier versions of Julia supported two types of lispy tuple programming:
one in which arguments shrink (e.g., via Base.tail) and the output gets
appended, and the other in which the result is appended to one of the
arguments (and therefore grows). For 0.7, inference has prioritized
either (1) proving that inference will converge or (2) exiting quickly.
Proving convergence is apparently much harder if some of the arguments
grow in size, so inference doesn't try very hard to get the right ansswer
anymore. Consequently inference was broken in our previous implementation.

This slight disadvantage is more than compensated by the fact that we
can now do inferrable arithmetic on type parameters, as exemplified here
by `ReshapedOneD{N,N-M}(arg)`.
@timholy
Copy link
Member Author

timholy commented Aug 17, 2018

OK, locally this passes all tests and AFAICT we are not commenting out any tests. Assuming this gets through CI, let's merge and tag.

@RalphAS, you may find the commit comment to 4a102c9 informative.

@timholy timholy merged commit b8179fd into master Aug 17, 2018
@timholy timholy deleted the teh/update0.7 branch August 17, 2018 03:05
@timholy
Copy link
Member Author

timholy commented Aug 17, 2018

Woot! Thanks to all who helped this happen.

@timholy timholy mentioned this pull request Aug 17, 2018
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