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

Update 1.0-dev with some commits from master (not in #271) #514

Merged
merged 8 commits into from
Apr 20, 2022

Conversation

lbenet
Copy link
Member

@lbenet lbenet commented Apr 2, 2022

This includes the latest commits in master, and #414. It still needs to upgrade changes introduced in #420.

Question: @Kolaru Did you think about including #420 into #271? Or why it shouldn't be ported?

@gwater Can you help clarifying the changes you proposed in #420, so we can port them? As a very short summary of related changes made in #271, there is no IntervalParameters struct anymore, and no extension of setprecision; everything is relying at the moment on setprecision and precision from Base.

This work was done together with @lucaferranti

@gwater
Copy link
Contributor

gwater commented Apr 2, 2022

@gwater Can you help clarifying the changes you proposed in #420, so we can port them? As a very short summary of related changes made in #271, there is no IntervalParameters struct anymore, and no extension of setprecision; everything is relying at the moment on setprecision and precision from Base.

Sure, no problem. The documentation for setprecision notes that behavior is undefined if setprecision is called from two threads simultaneously. In fact, I observed this when I experimented with multi-threading. The lock effectively prevents multi-threading any methods in IntervalArithmetic.jl which rely on MPFR (BigFloat)—this is mainly relevant for x^y operations but also some trigonometric and transcendental functions.

(Based on a cherry-pick from 8b8f44e)
@lbenet
Copy link
Member Author

lbenet commented Apr 6, 2022

Thanks @gwater for your answer and explanation! Could you have a look to abf8289, to see if it incorporates #420?

... instead of test_broken for a linear algebra test
@lucaferranti
Copy link
Member

@lbenet I think PR now fullfills its purpose, which was incorporating into the dev branch the changes in master that weren't there. I would suggest to merge this and investigate the test failure(s) in separate PRs

@gwater
Copy link
Contributor

gwater commented Apr 7, 2022

Thanks @gwater for your answer and explanation! Could you have a look to abf8289, to see if it incorporates #420?

Yes, that commit good to me. Only nit-pick would be that it's a little inconsistent with return statements. Should work fine though.

@Kolaru
Copy link
Collaborator

Kolaru commented Apr 7, 2022

Question: @Kolaru Did you think about including #420 into #271?

@lbenet Not explicitly. What I did is trust that if all tests pass, then I had taken everything into account properly while rebasing. Since thread safety is hard to test, it may have slipped through. It should definitely be ported though.

everything is relying at the moment on setprecision and precision from Base.

There are still a bunch of places where setprecision(Interval, ...) is used (mostly in the doc and in few tests). I think it would be nice to remove them before merging.

@lbenet
Copy link
Member Author

lbenet commented Apr 7, 2022

Yes, that commit good to me. Only nit-pick would be that it's a little inconsistent with return statements. Should work fine though.

I think it works fine, though I recall having seen those two return statements elsewhere, so I wrote them there. In any case, feel free to push into this PR your proposed changes.

@lbenet
Copy link
Member Author

lbenet commented Apr 7, 2022

There are still a bunch of places where setprecision(Interval, ...) is used (mostly in the doc and in few tests). I think it would be nice to remove them before merging.

Thanks for the observation! I'll clean those left-overs later today, or maybe tomorrow.

@lbenet
Copy link
Member Author

lbenet commented Apr 7, 2022

@Kolaru I checked the occurrences of setprecision in the docs. Since the docs (and the examples) have to be rewritten, I think it is better to leave that for another PR.

@gwater I changed bigequiv with respect to the return. Is this the nit-pick change you had in mind?

@@ -220,7 +220,7 @@ Create an equivalent `BigFloat` interval to a given `AbstractFloat` interval.
"""
function bigequiv(a::Interval{T}) where {T <: AbstractFloat}
lock(precision_lock) do
return setprecision(precision(T)) do # precision of T
setprecision(precision(T)) do # precision of T
return Interval{BigFloat}(a)
Copy link
Contributor

Choose a reason for hiding this comment

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

If we dont need the return statements above that we can drop this return statement, too.

@lbenet
Copy link
Member Author

lbenet commented Apr 20, 2022

@lbenet I think PR now fullfills its purpose, which was incorporating into the dev branch the changes in master that weren't there. I would suggest to merge this and investigate the test failure(s) in separate PRs

@lucaferranti Ok, I'll go ahead and merge this into the 1.0-dev branch. Some of the tests that are broken I'll address them in another PR.

@lbenet lbenet merged commit 4e918e0 into 1.0-dev Apr 20, 2022
@lbenet lbenet deleted the lb/master_to_1.0-dev branch April 20, 2022 17:32
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.

5 participants