Skip to content

Conversation

@p-zubieta
Copy link
Contributor

Looking at some packages invalidations, I found some introduced by this one. As far as I can see, much of these methods aren't supposed to work for things other than Numbers so I think these all should be harmless.

@p-zubieta p-zubieta requested a review from omus as a code owner September 23, 2020 23:53
@p-zubieta
Copy link
Contributor Author

I think I spoke too soon, I see that Intervals are expected, at least from the tests, to work with things like Dates. Maybe these really are "irreducible" invalidations, I'll have to think about it a bit more.

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.

Currently the only restriction the element type of Interval and AnchoredInterval is that the eltype must be comparable with isless. We may be able to get away with using promotion to convert a scalar (x) into a closed-interval (Interval(x, x)) which could work around the method invalidations by not having to define some of these methods. I suspect there will be a performance hit in doing this though.

src/endpoint.jl Outdated
Comment on lines 148 to 149
Base.:(==)(a::Number, b::Endpoint) = a == b.endpoint && isclosed(b)
Base.:(==)(a::Endpoint, b::Number) = b == a
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm pretty sure we can update this to be this safely (tests will show if that is the case):

Suggested change
Base.:(==)(a::Number, b::Endpoint) = a == b.endpoint && isclosed(b)
Base.:(==)(a::Endpoint, b::Number) = b == a
Base.:(==)(a::T, b::Endpoint{T}) where T = a == b.endpoint && isclosed(b)
Base.:(==)(a::Endpoint{T}, b::T) where T = b == a

I'm not sure these are actually used so we may be able to just drop them.

@codecov
Copy link

codecov bot commented Oct 1, 2020

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 75.76%. Comparing base (c37cbb7) to head (ca64ff6).
Report is 92 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #143   +/-   ##
=======================================
  Coverage   75.76%   75.76%           
=======================================
  Files          11       11           
  Lines         491      491           
=======================================
  Hits          372      372           
  Misses        119      119           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@p-zubieta
Copy link
Contributor Author

p-zubieta commented Oct 1, 2020

From code coverage, it looks like some of the new definitions are not used in the tests and we might get away with removing them. For others, it might be better to add code tests paths.

I'm not sure how does this look for someone more familiar with direct use cases of the package (I have only use it through other packages that have it as dependency).

@omus
Copy link
Collaborator

omus commented Oct 1, 2020

I'm looking into alternative approaches. I think we may be able to fix Base code such the + and - invalidations no longer occur (see #144). As for converting to a scalar I think we can define an alternative function for this behaviour.

@p-zubieta
Copy link
Contributor Author

p-zubieta commented Oct 1, 2020

That's great. I guess I will remove the arithmetic code changes. Yes, a custom scalar conversion function sounds sensible, but is probably better done in another PR.

@p-zubieta p-zubieta changed the title Restrict some arguments to Number to reduce invalidations Restrict arguments to Number in comparisons between scalars and EndPoint Oct 1, 2020
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.

2 participants