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

[.xts returns all data when i is not a valid date/time #96

Closed
joshuaulrich opened this issue May 21, 2015 · 2 comments
Closed

[.xts returns all data when i is not a valid date/time #96

joshuaulrich opened this issue May 21, 2015 · 2 comments
Labels
bug

Comments

@joshuaulrich
Copy link
Owner

@joshuaulrich joshuaulrich commented May 21, 2015

R> x <- xts(1:10, as.Date("2015-02-20")+0:9)
R> x['2012-02-30']
           [,1]
2015-02-20    1
2015-02-21    2
2015-02-22    3
2015-02-23    4
2015-02-24    5
2015-02-25    6
2015-02-26    7
2015-02-27    8
2015-02-28    9
2015-03-01   10

I thought the issue occured in/around lines 107-126 of .parseISO8601. At line 115, both s and e are NA, which causes them to be set to the min/max of the index values, respectively, when start and end aren't missing. My guess is that we could throw an error that x (the character date) isn't in the range if both s and e are NA...

But Jeff suggested that the fix should stay out of .parseISO8601. And that the issue is how NA is being handled in the subset itself. xts diverges from base R, in that we return nothing instead of all NAs, if using i = NA in the subset call. His take is that this case (either from or to are NA) should behave like subsetting with a date outside of the set (i.e. empty).

Thanks to Garrett See for the report.

@joshuaulrich joshuaulrich added the bug label May 21, 2015
@corwinjoy
Copy link
Contributor

@corwinjoy corwinjoy commented Jun 5, 2015

This makes me lean toward having the window function that I was proposing, have a base function that returns indexes. Essentially, subsetting with window should behave the same as subsetting with []. The idea would be to have subset process the range strings (as applicable) and call window_return_indexes. One wrinkle here is that the window.zoo function says that if start = NULL, this is interpreted as no constraint on the start date. (And similarly with end).

@corwinjoy
Copy link
Contributor

@corwinjoy corwinjoy commented Jun 8, 2015

I should clarify here. The point is not that window is automatically smarter (though it might help to have an NA check). The big point is there are some ambiguous cases - like what happens with an NA date - where reasonable people might disagree on the answer. Anyway - whatever standard is picked in subset for a range, I should make sure I follow it - and it may not be obvious. This says to me that I should make the effort to create a base window method that can be reused by subset to ensure consistency. So it means more work for me but is probably the right thing to do.

joshuaulrich added a commit that referenced this issue Jul 10, 2018
The prior commit did not address the case when the string used to
subset an xts object is only the range separator (i.e. "/" or "::").

Also add tests to ensure basic range-based subsets work, and that an
empty string also returns the entire data set.

See #96.
@joshuaulrich joshuaulrich added this to the Release 0.11-0 milestone Jul 30, 2018
joshuaulrich added a commit that referenced this issue Dec 26, 2018
This will cause a failure in 'R CMD check' in R-3.6.0. This was
introduced in 16bed0f as part of #96
(prevent subset from returning all data when 'i' is not a valid date-
time).

This should only be an issue when 'intervals' is length-1 and the first
3 logical comparisons are TRUE. So we only need to check whether the
first element of 'intervals' is an empty string.

Also add unit tests for basic functionality, and add some fuzz tests
too.

Fixes #280.
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.