Doc Improve. Replace timeseries_split unit-tests with a simple test #4070
Conversation
| @@ -6,16 +6,16 @@ RegressionLabels labels(f_labels) | |||
|
|
|||
| #![set parameters] | |||
| int num_subsets = 5 | |||
| int min_subset_size = 4 | |||
| int future_steps = 4 | |||
There was a problem hiding this comment.
I'd suggest future_offset but that is minor
| #![set parameters] | ||
|
|
||
| #![build subsets] | ||
| splitting.build_subsets() | ||
| #![build subsets] | ||
|
|
||
| #![generate subsets and inverse (aka test labels and train labels)] | ||
| IntVector test_labels_indices = splitting.generate_subset_indices(1) | ||
| #![generate subsets and inverse (aka vaildation set and train set)] |
There was a problem hiding this comment.
This is a tag for the cookbooks, not free form comment. So pls make it super short (see other examples)
| } | ||
|
|
||
| void CTimeSeriesSplitting::build_subsets() | ||
| { | ||
| REQUIRE(m_num_subsets > 0, "Number of subsets should be greater than 0."); |
|
|
||
| /* filling current with indices on right end */ | ||
| /* filling current with indices on right side */ | ||
| for (auto k = split_index; k < indices.vlen; ++k) |
There was a problem hiding this comment.
auto k : Range(indices.vlen) but it is minor
| REQUIRE(min_size > 0, "Minimum subset size should be atleast 1.") | ||
| /* future_steps should be less than the difference between number of labels | ||
| * and split index of second last fold */ | ||
| index_t future_steps_upperbound = |
There was a problem hiding this comment.
max_future_steps
or better max_future_offset
| * indices greater than a split index. The split indices are \f$ c[N/K] \f$ | ||
| /** @brief Implements a timeseries splitting strategy for cross-validation, | ||
| * respecting time. | ||
| * Each fold splits timeseries into train (subset_inverse) and validation |
There was a problem hiding this comment.
pls remove the (subset_inverse) here. This is documented in the base class
The newlines are awkwards, could you remove them?
| * (subset) set. | ||
| * Train set contains indices less than a split index | ||
| * and validation set contains rest of the indices. | ||
| * The split indices are \f$ C*floor(N/K) \f$ |
There was a problem hiding this comment.
in latex math, you don't use * for multiplication but \cdot or just nothing
| splitting->build_subsets(); | ||
|
|
||
| for (index_t i = 0; i < num_subsets; ++i) | ||
| labels = new CRegressionLabels(num_labels); |
There was a problem hiding this comment.
labels = some(num_labels); and then you dont have to SG_UNREF later
| SG_UNREF(splitting); | ||
| for (auto i : range(12)) | ||
| { | ||
| EXPECT_EQ(train_indices[i], i); |
There was a problem hiding this comment.
yes this is a better test in fact!
|
@Sahil333 any update on this? |
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
|
This issue is now being closed due to a lack of activity. Feel free to reopen it. |
No description provided.