Sn/ignore bad dates

[snakch]

#14

The purpose of this PR is to add a bad_date optional argument to the partition function which allows the user to specify date that should be dropped. This should be useful in a number of downstream applications.

[oxinabox]

I suggest renaming this to exclude and making default arg typed.

Here and everywhere else

Suggested change

	function Iterators.partition(dates::StepRange{Date, Day}, s::PeriodicSelector; bad_dates=[])
	function Iterators.partition(dates::StepRange{Date, Day}, s::PeriodicSelector; exclude=Date[])

[oxinabox]

Not really a performance issue here, but in general best practice is:

Suggested change

	@test !any(is_bad.(validation))
	@test !any(is_bad.(holdout))
	@test !any(is_bad, validation)
	@test !any(is_bad, holdout)

as it avoids allocations

[oxinabox]

Suggested change

	@testset "1 week period, 1 day stride, remove the first two holdout dates" begin
	@testset "1 week period, 1 day stride, remove the first holdout dates" begin

[mzgubic]

Why is this approach better than adding another selector (as suggested in #14)?

[oxinabox]

this is equivelent to

map(d->filter(!in(bad_dates), d), partition(dates, selector))

I wonder if we might want to instead define a

filter_partitions(by, parts) = map(d->filter(by, d), parts)

which could be used for this and other things as well

[snakch]

Why is this approach better than adding another selector (as suggested in #14)?

I considered doing another selector, but modifying partition felt more natural, since excluding dates is something one might want regardless of date selection technique.
On the downside, it is definitely more invasive than defining a new selector, so for other services that use DateSelectors your suggestion might be best overall. Happy to rework this entirely if we decide to go this way

this is equivelent to

map(d->filter(!in(bad_dates), d), partition(dates, selector))

I wonder if we might want to instead define a

filter_partitions(by, parts) = map(d->filter(by, d), parts)

which could be used for this and other things as well

Yes this feels like possibly a good in-between? Again the issue is that client modules that use partition might need to be redefined to allow filter_partitions, so it's maybe a bit more disruptive.

[mzgubic]

I considered doing another selector, but modifying partition felt more natural, since excluding dates is something one might want regardless of date selection technique.

Oh, I assumed that selectors are composable, but that does not seem to be the case.

Again the issue is that client modules that use partition might need to be redefined to allow filter_partitions, so it's maybe a bit more disruptive.

Hmm, I'm not sure I understand this entirely. Would it not be possible to just do:

validation, holdout = filter_partitions(!in(excluded_dates), partition(...))

in client code?

[snakch]

Oh, I assumed that selectors are composable, but that does not seem to be the case. I'll open an issue.

They are, i'd actually misread the issue. I think that solution is much better after some thought, so I'll overhaul the PR in a bit. No need to open another issue!