DatetimeEncoder fixes

[LeoGrin]

• Fix BUG example 03 breaks with exact_until='microsecond' #741 Remove millisecond extraction in DatetimeEncoder (Turns out you pandas' DatetimeIndex don't have a millisecond attribute.)
• Fix BUG DateTimeEncoder fails when extract_until is "year" or "month" #745 Stop using the floor function and simplify the logic to check if we need total_time
• Close FEAT Getting only the time since epoch from the DateTimeEncoder #744 Adds a None option for extract_until to only extract total_time.

@jeromedockes

[Vincent-Maladiere]

Thanks @LeoGrin for this bug fix. This might help fix #741 afterward.
Overall, I'm concerned with the quality of this estimator as we have other bugs like #746. This means that we didn't test it enough.

[Vincent-Maladiere]

Hey @LeoGrin, I'd advocate not to change the scope of PRs once started because it's confusing to review. To fix new issues, let's open new PRs instead :)

Dismissing my approval since the scope of the PR has changed.

[Vincent-Maladiere]

I think returning the total number of seconds since Epoch when self.extract_until=None is misleading for the user.
I'd rather have self.extract_until=None as a no-op (not returning anything), and have an additional keyword init parameter like self.seconds_since_epoch.
What do you both think?

Otherwise, I haven't specific remarks on the PR itself, but I plan to refactorize a lot of the DatetimeEncoder implementation that I'm not super happy with in a subsequent PR.

[jeromedockes]

I think returning the total number of seconds since Epoch when `self.extract_until=None` is misleading for the user. I'd rather have `self.extract_until=None` as a no-op (not returning anything), and have an additional keyword init parameter like `self.seconds_since_epoch`. What do you both think?

I agree, that seems better. If we don't want to add a parameter we could even always return the seconds since epoch in addition to any other features the user requests

Otherwise, I haven't specific remarks on the PR itself, but I plan to refactorize a lot of the DatetimeEncoder implementation that I'm not super happy with in a subsequence PR.

I agree the DateTimeEncoder could use some cleanup. For example it re-parses the whole date column to extract each feature. Also the DateTimeEncoder and the TableVectorizer both parse date strings, and they do it in different ways so it may be worth thinking about their respective responsibilities and doing some refactoring.

[LeoGrin]

I think returning the total number of seconds since Epoch when self.extract_until=None is misleading for the user.
I'd rather have self.extract_until=None as a no-op (not returning anything), and have an additional keyword init parameter like self.seconds_since_epoch.
What do you both think?

Yes, I think that's a good idea. Thinking about it a bit more, it would better for the shape of the output to not depend on the data, for instance if we want to use this inside a grid search, so it may be better to get rid of the logic we have now (check if columns are constant).

[jeromedockes]

Thinking about it a bit more, it would better for the shape of the output to not depend on the data, for instance if we want to use this inside a grid search, so it may be better to get rid of the logic we have now (check if columns are constant).

I think that's a good idea. Dropping constant columns can be done later in the pipeline with a VarianceThreshold transformer from scikit-learn. I guess the idea was to avoid adding eg an hour column full of 0 when the column we are transforming contains dates not times. But for that it may be better to rely on the date format detected when parsing the dates than to check the variance a posteriori

[Vincent-Maladiere]

I guess the idea was to avoid adding eg an hour column full of 0 when
the column we are transforming contains dates not times. But for that it
may be better to rely on the date format detected when parsing the dates
than to check the variance a posterior

Alternatively, we could let the user switch between these two behaviors with an additional drop_if_constant keyword argument.

I think that's a good idea. Dropping constant columns can be done later
in the pipeline with a VarianceThreshold transformer from scikit-learn.

We definitely need to add the VarianceThreshold estimator to the example!

[Vincent-Maladiere]

Let's change the behavior of extract_until=None to be a no-op and add an extra add_total_seconds_since_epoch or add_seconds_since_epoch keyword parameter (default True) as discussed :)

[Vincent-Maladiere]

Suggested change

	* Fix bugs which was triggered when `extract_until` was "year", "month", "microseconds"
	* Fix bugs which where triggered when `extract_until` was "year", "month", "microseconds"

[Vincent-Maladiere]

Let's merge so that we can do the refactoring soon.