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

Deprecate convert for closed intervals in favor of only #145

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

pabloferz
Copy link
Contributor

Another take on #144 as suggested in #143. I don't know if this is the best name for this function nor how the presence of the deprecation affects removing the method. But throwing the idea here anyway to keep the ball rolling.

@pabloferz pabloferz requested a review from omus as a code owner October 7, 2020 04:24
@pabloferz pabloferz force-pushed the pz/to_scalar branch 2 times, most recently from a8fa689 to f3165b3 Compare October 8, 2020 03:42
@codecov
Copy link

codecov bot commented Oct 8, 2020

Codecov Report

Merging #145 (9167160) into master (e04f9d4) will increase coverage by 0.20%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #145      +/-   ##
==========================================
+ Coverage   81.73%   81.93%   +0.20%     
==========================================
  Files          11       11              
  Lines         624      631       +7     
==========================================
+ Hits          510      517       +7     
  Misses        114      114              
Impacted Files Coverage Δ
src/deprecated.jl 14.87% <ø> (ø)
src/anchoredinterval.jl 99.15% <100.00%> (+0.02%) ⬆️
src/interval.jl 96.41% <100.00%> (+0.04%) ⬆️

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 e04f9d4...9167160. Read the comment docs.

@pabloferz
Copy link
Contributor Author

@omus was something like this what you had in mind here?

@nalimilan
Copy link

Maybe making this a method of only would make sense?

@KristofferC
Copy link
Contributor

KristofferC commented Mar 22, 2021

Bump, I'm seeing thousands of invalidations less with this PR (for a big package I am looking at).

The next big one is

Base.:(==)(a, b::Endpoint) = a == b.endpoint && isclosed(b)
Base.:(==)(a::Endpoint, b) = b == a

givinb 2.6k invalidations.

@pabloferz
Copy link
Contributor Author

For the latter there is #143.

@omus
Copy link
Collaborator

omus commented Mar 24, 2021

@omus was something like this what you had in mind here?

Yes, this is. I was thinking of using an alternative function name like scalar but only seems quite reasonable

Copy link
Collaborator

@omus omus left a comment

Choose a reason for hiding this comment

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

With a few changes and a rebase to get the CI working I think we can get this merged

src/anchoredinterval.jl Show resolved Hide resolved
src/anchoredinterval.jl Outdated Show resolved Hide resolved
src/interval.jl Outdated Show resolved Hide resolved
@pabloferz pabloferz force-pushed the pz/to_scalar branch 4 times, most recently from 7e04521 to 44dece7 Compare January 17, 2022 18:27
@pabloferz
Copy link
Contributor Author

The failing tests are unrelated, but I' not sure about the docs failure.

@pabloferz pabloferz changed the title Replace convert for scalars Deprecate convert for closed intervals in favor of only Jan 25, 2022
Copy link
Collaborator

@omus omus left a comment

Choose a reason for hiding this comment

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

Only some minor things left here

src/interval.jl Outdated Show resolved Hide resolved
test/interval.jl Outdated Show resolved Hide resolved
src/interval.jl Outdated Show resolved Hide resolved
src/interval.jl Show resolved Hide resolved
@pabloferz pabloferz force-pushed the pz/to_scalar branch 3 times, most recently from 52db1b2 to 03a4ecd Compare February 19, 2022 15:48
@pabloferz pabloferz requested a review from omus February 19, 2022 19:12
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.

4 participants