Skip to content

Conversation

@rok
Copy link
Member

@rok rok commented Feb 9, 2022

This is to resolve ARROW-14825.

@github-actions
Copy link

github-actions bot commented Feb 9, 2022

@rok
Copy link
Member Author

rok commented Feb 9, 2022

Would us_year be a better name as it's consistent with already present us_week name?

@lidavidm
Copy link
Member

lidavidm commented Feb 9, 2022

us_year seems to better in that regard. (Looking it up - it turns out "epiweek" means something different in the US vs the rest of the world? Oof.)

@rok
Copy link
Member Author

rok commented Feb 9, 2022

us_year seems to better in that regard. (Looking it up - it turns out "epiweek" means something different in the US vs the rest of the world? Oof.)

Well we're looking to mirror lubridate's epiyear for our R implementation, see ARROW-14824.
We could also call it us_epiyear to preventively disambiguate.

@lidavidm
Copy link
Member

lidavidm commented Feb 9, 2022

us_epiyear makes sense.

@rok
Copy link
Member Author

rok commented Feb 9, 2022

@lidavidm Switched to us_epiyear.

@lidavidm
Copy link
Member

lidavidm commented Feb 9, 2022

Ah, the function doc description needs to be line-wrapped.

@rok
Copy link
Member Author

rok commented Feb 9, 2022

Fixed.

@jorisvandenbossche
Copy link
Member

Should there then also be an equivalent epiweek kernel?

@rok
Copy link
Member Author

rok commented Feb 10, 2022

@jorisvandenbossche we currently have us_week and iso_week. us_week is us_epiyear-like. We kind of have two camps here - iso and us/epi. Very much open to suggestions on naming here.

@jorisvandenbossche
Copy link
Member

OK, I see. For consistency with us_week, could then also call it us_year? Or is that too vague?

@rok
Copy link
Member Author

rok commented Feb 10, 2022

I don't mind either way. US year seems good because it's consistent with the ISO year definition - year starts with week 1 and it's either from Monday (ISO) for Sunday (US). On the other hand it is inventing a new name :).

@jorisvandenbossche
Copy link
Member

The "us_week" was also a new invented name at the time? (or is that used elsewhere)

@rok
Copy link
Member Author

rok commented Feb 10, 2022

The "us_week" was also a new invented name at the time? (or is that used elsewhere)

So was assume_timezone. I suppose we've crossed that threshold. I'll change to us_year.

@rok
Copy link
Member Author

rok commented Feb 10, 2022

@jorisvandenbossche done.

@lidavidm lidavidm closed this in 54f6c03 Feb 10, 2022
@ursabot
Copy link

ursabot commented Feb 10, 2022

Benchmark runs are scheduled for baseline = cce55b4 and contender = 54f6c03. 54f6c03 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished ⬇️0.0% ⬆️0.0%] ec2-t3-xlarge-us-east-2
[Finished ⬇️2.56% ⬆️0.0%] test-mac-arm
[Failed ⬇️0.71% ⬆️0.0%] ursa-i9-9960x
[Finished ⬇️0.43% ⬆️0.04%] ursa-thinkcentre-m75q
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

@rok
Copy link
Member Author

rok commented Feb 10, 2022

Thanks @lidavidm @jorisvandenbossche !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants