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 0.6 depwarns #573

Merged
merged 1 commit into from
Jan 15, 2017
Merged

Fix 0.6 depwarns #573

merged 1 commit into from
Jan 15, 2017

Conversation

yuyichao
Copy link
Collaborator

This fixes all of the test errors and most of the depwarns on master. Passing tests on master requires JuliaIO/FileIO.jl#94 and some depwarns are fixed by JuliaAttic/FactCheck.jl#80

The depwarns that are not fixed are:

  • Extending .+, .== etc

    Might need @compat support and might also need to decide what's the expected return type with loop fusion

  • @test_approx_eq

    It seems to behave differently wrt NaN?

  • reduce_dims

    Might want to go to Compat.jl

@yuyichao
Copy link
Collaborator Author

Sign.... 0.4 support might require doing something about broadcast shape in compat.....

@yuyichao
Copy link
Collaborator Author

Also for 0.5....

@yuyichao yuyichao force-pushed the yyc/0.6 branch 2 times, most recently from bac4a14 to 8316560 Compare January 12, 2017 00:35
@yuyichao
Copy link
Collaborator Author

.... End up working around it in the tests since I'm too lazy (and not sure how) to fix those properly on old julia versions.

There are a few pretty ugly or evil workaround but luckily they are all in the tests....

Local tests passes on 0.4 and 0.5...

mapi = ScaleAutoMinMax()
A = [100,550,1000]
@chk map(mapi, A) @compat UFixed8.([0.0,0.5,1.0])
@chk map(mapi, A) UFixed8.([0.0,0.5,1.0])
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah, this one was a accidental change. I can change it back if someone feels very strongly about it... Otherwise I'll just leave it like this....

Copy link
Member

Choose a reason for hiding this comment

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

It's fine.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.06%) to 78.708% when pulling 271cc34 on yyc/0.6 into 1b3536e on master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.06%) to 78.708% when pulling 271cc34 on yyc/0.6 into 1b3536e on master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.06%) to 78.708% when pulling 271cc34 on yyc/0.6 into 1b3536e on master.

@yuyichao
Copy link
Collaborator Author

All test errors on master are from FileIO and should be fixed by JuliaIO/FileIO.jl#94

@yuyichao
Copy link
Collaborator Author

This does not regress on 0.4 and 0.5 and since the FileIO PR is merged (though not tagged) I'll merge this during the weekend if there's no more comments.

Copy link
Member

@timholy timholy left a comment

Choose a reason for hiding this comment

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

I had been assuming you were waiting for FileIO to be tagged, but it looks like no one has submitted a tag PR. Merge whenever you wish.

@fact all((cos.(agphase) .* cos.(aorient) .+
sin.(agphase) .* sin.(aorient) .< EPS) |
((agphase .== 0.0) & (aorient .== 0.0))) --> true
@fact all((|).(cos.(agphase) .* cos.(aorient) .+
Copy link
Member

Choose a reason for hiding this comment

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

I'm all in favor of the improvements in efficiency that the new capabilities bring, but it's Interesting how this is a step backwards in terms of readability. If one were to abandon 0.4 I suppose we could use a generator.

Copy link
Member

Choose a reason for hiding this comment

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

Not that I'm proposing you should do that. Coming in #542.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The issues that affect 0.4 are JuliaLang/Compat.jl#306 and JuliaLang/Compat.jl#282. If JuliaLang/Compat.jl#306 is merged and used here, the only necessary change should be replacing | with .|.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

(and also & with .& .....)

Copy link
Member

Choose a reason for hiding this comment

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

To clarify, I wasn't complaining about this specific use, just making a more general language observation. I'm fine with how you've done it here.

mapi = ScaleAutoMinMax()
A = [100,550,1000]
@chk map(mapi, A) @compat UFixed8.([0.0,0.5,1.0])
@chk map(mapi, A) UFixed8.([0.0,0.5,1.0])
Copy link
Member

Choose a reason for hiding this comment

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

It's fine.

@yuyichao
Copy link
Collaborator Author

I had been assuming you were waiting for FileIO to be tagged

Not really, since this shouldn't create regression without it and doesn't really depend on anything there. The two are basically independent fixes....

@timholy
Copy link
Member

timholy commented Jan 14, 2017

Well, like I said, I approve, so merge when you wish. Thanks for the contribution!

@yuyichao yuyichao merged commit 518d168 into master Jan 15, 2017
@yuyichao yuyichao deleted the yyc/0.6 branch January 15, 2017 00:36
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