Skip to content

reverse deprecation of find_root, make const#603

Closed
sbromberger wants to merge 1 commit intoJuliaCollections:masterfrom
sbromberger:sbromberger/change-depwarn
Closed

reverse deprecation of find_root, make const#603
sbromberger wants to merge 1 commit intoJuliaCollections:masterfrom
sbromberger:sbromberger/change-depwarn

Conversation

@sbromberger
Copy link
Copy Markdown
Contributor

@sbromberger sbromberger commented Apr 3, 2020

No description provided.

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

mlubin commented Apr 3, 2020

LGTM, thanks for the fix!

@sbromberger
Copy link
Copy Markdown
Contributor Author

sbromberger commented Apr 3, 2020

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
Copy Markdown

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
Copy Markdown
Contributor Author

sbromberger commented Apr 5, 2020

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
Copy Markdown
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
Copy Markdown
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
Comment thread src/DataStructures.jl
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
Copy Markdown
Member

@eulerkochy eulerkochy Apr 6, 2020

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
Copy Markdown
Member

@oxinabox oxinabox Apr 11, 2020

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Comment thread src/deprecations.jl
@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
Copy Markdown
Member

@oxinabox oxinabox Apr 11, 2020

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
Copy Markdown
Member

oxinabox commented Apr 11, 2020

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
Copy Markdown
Member

oxinabox commented Apr 12, 2020

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