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

Allow passing vectors of dates #25

Merged
merged 2 commits into from
Dec 7, 2022
Merged

Allow passing vectors of dates #25

merged 2 commits into from
Dec 7, 2022

Conversation

rofinn
Copy link
Member

@rofinn rofinn commented Nov 29, 2022

This should be non-breaking as we're just loosening some of the existing restrictions. Most of the code already worked for abstract vectors, but now we hard code step(dates) as Day(1) and throw an ArgumentError if we're passed a different step size. Needed to update a single pre-existing test which didn't allow vectors and switched the step size tests to check the ArgumentError vs MethodError.

Closes #18

Also may satisfy the requirements for #14 and #17. Although, maybe having a separate PR for an ExcludeSelector would be useful?

@codecov
Copy link

codecov bot commented Nov 29, 2022

Codecov Report

Merging #25 (0dc394a) into main (845b79f) will increase coverage by 0.95%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main      #25      +/-   ##
==========================================
+ Coverage   89.04%   90.00%   +0.95%     
==========================================
  Files           5        5              
  Lines          73       80       +7     
==========================================
+ Hits           65       72       +7     
  Misses          8        8              
Impacted Files Coverage Δ
src/common.jl 80.64% <ø> (ø)
src/NoneSelector.jl 100.00% <100.00%> (ø)
src/PeriodicSelector.jl 100.00% <100.00%> (ø)
src/RandomSelector.jl 91.30% <100.00%> (+0.82%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@oxinabox
Copy link
Member

  1. Can you tell me more about why we want this
  2. I worry that if not careful we will lose the key invariant feature that wether or not a date is selected should not be a property of the dates offerred but rather of only of the parameters of the selector
  3. isn't excluding dates, and selecting a subset of dates easily enough done in post processing via setdiff and intersect?

@rofinn
Copy link
Member Author

rofinn commented Nov 29, 2022

Can you tell me more about why we want this?

Mostly just that it seems overly restrictive?

I worry that if not careful we will lose the key invariant feature that whether or not a date is selected should not be a property of the dates offered but rather of only of the parameters of the selector

That feels like an assertion that should be tested for each selector regardless of whether we allow vectors.

Isn't excluding dates, and selecting a subset of dates easily enough done in post processing via setdiff and intersect?

True, but we already pass around a selector as a kind of declarative representation of what dates we want. I think eventually allowing nesting or composition of selectors with an explicit inclusion/exclusion could be a useful way of declaratively specifying the dates considered for a project. I suppose we could choose not to loosen the types here and simply support wrapping directly and have a ExcludeSelector which does that post-processing on the selector outputs?

@rofinn rofinn merged commit f229f75 into main Dec 7, 2022
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.

partition to accept a vector of dates
2 participants