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

Should constructors initialized with `NA` set all elements to `NA`? #869

Closed
DavisVaughan opened this issue Mar 19, 2020 · 3 comments
Closed

Should constructors initialized with `NA` set all elements to `NA`? #869

DavisVaughan opened this issue Mar 19, 2020 · 3 comments
Milestone

Comments

@DavisVaughan
Copy link
Member

@DavisVaughan DavisVaughan commented Mar 19, 2020

I find it a little strange that if you add an NA, or initialize with NA, that it doesn't set all of the slots to NA. I feel like this implies that there are multiple "types" of missing Period objects. It might help internal consistency if NA propagated to all slots. This would formalize a missing Period object as one where all slots are NA

library(lubridate)

days(NA_real_)
#> [1] NA

str(days(NA_real_))
#> Formal class 'Period' [package "lubridate"] with 6 slots
#>   ..@ .Data : num NA
#>   ..@ year  : num 0
#>   ..@ month : num 0
#>   ..@ day   : num NA
#>   ..@ hour  : num 0
#>   ..@ minute: num 0

days(NA_real_) + years(1)
#> [1] NA

days(1) + years(NA_real_)
#> [1] NA

str(days(NA_real_) + years(1))
#> Formal class 'Period' [package "lubridate"] with 6 slots
#>   ..@ .Data : num NA
#>   ..@ year  : num 1
#>   ..@ month : num 0
#>   ..@ day   : num NA
#>   ..@ hour  : num 0
#>   ..@ minute: num 0

str(days(1) + years(NA_real_))
#> Formal class 'Period' [package "lubridate"] with 6 slots
#>   ..@ .Data : num NA
#>   ..@ year  : num NA
#>   ..@ month : num 0
#>   ..@ day   : num 1
#>   ..@ hour  : num 0
#>   ..@ minute: num 0

This is what I expect internally:

library(lubridate)
#> 
#> Attaching package: 'lubridate'
#> The following object is masked from 'package:base':
#> 
#>     date

period()[NA_real_]
#> [1] NA

str(period()[NA_real_])
#> Formal class 'Period' [package "lubridate"] with 6 slots
#>   ..@ .Data : num NA
#>   ..@ year  : num NA
#>   ..@ month : num NA
#>   ..@ day   : num NA
#>   ..@ hour  : num NA
#>   ..@ minute: num NA
@vspinu
Copy link
Member

@vspinu vspinu commented Mar 19, 2020

I thought about this briefly recently but didn't implement because of the efficiency and simplicity reasons. But I agree, for the sake of things like minute(days(NA)) it might be worth implementing it.

What do you need this internal "consistency" for? I am just afraid that syncronizing them might be a rabbit hole and might not be done properly in all situations.

@DavisVaughan
Copy link
Member Author

@DavisVaughan DavisVaughan commented Mar 20, 2020

I'm working on vctrs methods for lubridate, and was adding a test that implicitly required period()[NA_integer_] to be identical to period(NA_integer_)

@vspinu
Copy link
Member

@vspinu vspinu commented Mar 21, 2020

Hmm, interesting corner case. Ok, let's add this in the constructor then. It should not harm, and it's the right thing to do anyhow.

@vspinu vspinu added this to the 1.7.8 milestone Mar 21, 2020
@vspinu vspinu closed this in 1c55579 Mar 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
2 participants
You can’t perform that action at this time.