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

Support signbit(::Dates.Period) #51073

Merged
merged 1 commit into from
Feb 15, 2024
Merged

Support signbit(::Dates.Period) #51073

merged 1 commit into from
Feb 15, 2024

Conversation

jishnub
Copy link
Contributor

@jishnub jishnub commented Aug 27, 2023

signbit(x) is defined to

Return true if the value of the sign of x is negative, otherwise false.

Since sign is defined for a Period, it makes sense for signbit to be defined as well. After this,

julia> sign(Day(1))
1

julia> signbit(Day(1))
false

@jishnub jishnub added dates Dates, times, and the Dates stdlib module stdlib Julia's standard library labels Aug 27, 2023
@LilithHafner
Copy link
Member

Seems somewhat reasonable, what's the motivating usecase for adding this feature?

@jishnub
Copy link
Contributor Author

jishnub commented Oct 9, 2023

I don't have a very compelling use case, but I had noticed this when trying to construct an infinite range of dates (i.e. an arbitrarily long sequence of dates starting at a specified point). The current implementation of ranges in InifniteArrays uses signbit, so having this defined improves composability.

Copy link
Member

@LilithHafner LilithHafner left a comment

Choose a reason for hiding this comment

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

Sure, why not

@jishnub
Copy link
Contributor Author

jishnub commented Oct 10, 2023

@halleysfifthinc Could you elaborate why you dislike this proposal?

@halleysfifthinc
Copy link
Contributor

This is completely my own interpretation, since the documentation doesn't explicitly state this, but I understood signbit to only/mainly apply to primitive types (returning the actual sign bit), and sign should be used for all other sign needs (including extending for non-primitive types).

@jishnub
Copy link
Contributor Author

jishnub commented Oct 11, 2023

signbit is already defined for e.g. Rational, which isn't really a primitive type, although it's still a number. There's also a signbit method for Real, so non-primitive number types get the fallback implementation. Now, a Period isn't a number, but it's documented as a simple wrapper for an Int, so perhaps it's reasonable to extend the definition?

That being said, I understand the concern, but I wonder if we want to be strict about this.

@fingolfin fingolfin closed this Feb 15, 2024
@fingolfin fingolfin reopened this Feb 15, 2024
Copy link
Contributor

@fingolfin fingolfin left a comment

Choose a reason for hiding this comment

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

LGTM

@fingolfin fingolfin added the merge me PR is reviewed. Merge when all tests are passing label Feb 15, 2024
@LilithHafner LilithHafner changed the title Signbit for Period Support signbit(x::Dates.Period) Feb 15, 2024
@LilithHafner LilithHafner changed the title Support signbit(x::Dates.Period) Support signbit(::Dates.Period) Feb 15, 2024
@LilithHafner LilithHafner merged commit e460d35 into master Feb 15, 2024
11 of 13 checks passed
@LilithHafner LilithHafner deleted the jishnub/periodsignbit branch February 15, 2024 15:55
@giordano giordano removed the merge me PR is reviewed. Merge when all tests are passing label Feb 15, 2024
tecosaur pushed a commit to tecosaur/julia that referenced this pull request Mar 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dates Dates, times, and the Dates stdlib module stdlib Julia's standard library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants