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.

Sign up for GitHub

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

Jump to bottom

reverse deprecation of find_root, make const #603

Closed
wants to merge 1 commit into from
Closed

reverse deprecation of find_root, make const #603

wants to merge 1 commit into from

Conversation

sbromberger
Copy link
Contributor

No description provided.

@sbromberger sbromberger mentioned this pull request Apr 3, 2020
Merged
@mlubin
Copy link
Contributor

mlubin commented Apr 3, 2020

LGTM, thanks for the fix!

@sbromberger
Copy link
Contributor Author

There were several ways of doing this, but I chose one that should be the least intrusive and the easiest to properly deprecate in the future.

@codecov
Copy link

codecov bot commented Apr 3, 2020 •
edited
Loading

Codecov Report

Merging #603 into master will decrease coverage by 0.19%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##           master     #603     +/-   ##
=========================================
- Coverage   95.04%   94.85%   -0.2%     
=========================================
  Files          33       33             
  Lines        2825     2835     +10     
=========================================
+ Hits         2685     2689      +4     
- Misses        140      146      +6
Impacted Files Coverage Δ
src/DataStructures.jl 100% <ø> (ø) ⬆️
src/delegate.jl 88% <0%> (-3.67%) ⬇️
src/heaps/arrays_as_heaps.jl 91.66% <0%> (-2.62%) ⬇️
src/circ_deque.jl 98% <0%> (-2%) ⬇️
src/multi_dict.jl 61.19% <0%> (-1.89%) ⬇️
src/default_dict.jl 87.5% <0%> (-1.87%) ⬇️
src/accumulator.jl 92.59% <0%> (-1.16%) ⬇️
src/sorted_multi_dict.jl 92.1% <0%> (-0.82%) ⬇️
src/heaps/mutable_binary_heap.jl 92.43% <0%> (-0.79%) ⬇️
src/sorted_set.jl 92.81% <0%> (-0.52%) ⬇️
... and 1 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 774a2a8...7e642e0. Read the comment docs.

@oxinabox oxinabox mentioned this pull request Apr 4, 2020
Merged
@sbromberger
Copy link
Contributor Author

ping @oxinabox @eulerkochy - this is intended to ameliorate the issues with JuMP.

@pkj-m pkj-m mentioned this pull request Apr 6, 2020
Open
@mlubin
Copy link
Contributor

mlubin commented Apr 6, 2020

Is there a plan to merge and tag this or should we figure out our own workaround in JuMP? JuliaLang/julia#35362 is a good long-term solution but doesn't fix the immediate issue.

eulerkochy
eulerkochy approved these changes Apr 6, 2020
View reviewed changes
Copy link
Member

@eulerkochy eulerkochy left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Extremely sorry for the delay. LGTM!

eulerkochy
eulerkochy reviewed Apr 6, 2020
View reviewed changes
src/DataStructures.jl
@@ -69,6 +69,7 @@ module DataStructures
include("accumulator.jl")
include("classified_collections.jl")
include("disjoint_set.jl")
const find_root = find_root! # remove when deprecating find_root. See deprecations.jl
Copy link
Member

Choose a reason for hiding this comment

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

just put this in disjoint_sets.jl

Copy link
Member

Choose a reason for hiding this comment

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

I think i would put this in deprecations.jl
Because that is the file I check everytime I am about to make a minor release.

eulerkochy
eulerkochy requested changes Apr 6, 2020
View reviewed changes
Copy link
Member

@eulerkochy eulerkochy left a comment

Choose a reason for hiding this comment

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

one small change reqd.

@eulerkochy eulerkochy requested a review from oxinabox April 6, 2020 14:09
@oxinabox
Copy link
Member

oxinabox commented Apr 6, 2020

I think this is the only way forward.
I intensely do not love this fact.

What is our exit plan?

  • Option 1) not deprecate this until v0.18, and not remove it to v0.19?
  • Option 2) remove without deprecation in 0.18
  • Option 3) hold-off deprecating it til wherever in JuMP land updates to use new version, then we deprecate it properly, in a 0.17.x release.
  • Option 4) deprecate in last release before we make release of 0.18

I am going to keep out of this statements about how SemVer is supposed to work from this discussion, to find the practical solution.
I would request others do the same, and refrain from statements e.g. about if this was a breaking change or not.

@mlubin
Copy link
Contributor

mlubin commented Apr 6, 2020 •
edited by oxinabox
Loading

(option 5)
You could consider a slight variant of option 2: remove it in 0.18 with a helpful error message instead of a deprecation warning.

oxinabox
oxinabox reviewed Apr 11, 2020
View reviewed changes
src/deprecations.jl
@@ -1,4 +1,4 @@
@deprecate front(x) first(x)
@deprecate back(x) last(x)
@deprecate top(x) first(x)
@deprecate find_root find_root! # 2020-03-31
# @deprecate find_root find_root! # 2020-03-31 - reimplement with new minor version
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
# @deprecate find_root find_root! # 2020-03-31 - reimplement with new minor version
# @deprecate find_root find_root! # 2020-03-31 - deprecate in v0.18, or when Julia 1.5 is released.

@oxinabox
Copy link
Member

Ok, my conclusion is to either: deprecate it in 0.18, or once julia 1.5 is released, which ever comes out first.

so I think I am am happy with this.
a few suggestions then we can do it.

@ccoffrin ccoffrin mentioned this pull request Apr 12, 2020
Merged
@oxinabox
Copy link
Member

closed in #604

@oxinabox oxinabox closed this Apr 12, 2020
@oxinabox oxinabox mentioned this pull request Apr 12, 2020
Closed
3 tasks
@oxinabox oxinabox mentioned this pull request Jul 11, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@oxinabox oxinabox oxinabox left review comments

@eulerkochy eulerkochy eulerkochy requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@sbromberger @mlubin @oxinabox @eulerkochy