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

Add WalkForwardTimeSeriesCrossValidation #18

Merged
merged 8 commits into from Aug 18, 2019

Conversation

@Eric2Hamel
Copy link
Contributor

commented Jul 25, 2019

  • Add WalkForwardTimeSeriesCrossValidation
  • Unittest split method from WalkForwardTimeSeriesCrossValidation

Eric2Hamel added some commits Jul 25, 2019

- Add NumpyConcatenateOnTimeStep
- Add WalkForwardTimeSeriesCrossValidation
- Solve a bug in the slicing where the comparaison was taken on the l…
…en(data_inputs) and not data_inputs.shape[1].

- Add a unittest for WalkForwardTimeSeriesCrossValidation
@cla-bot

This comment has been minimized.

Copy link

commented Jul 25, 2019

Thank you for contributing!

We detected that this might be your first contribution to a Neuraxio open-source project. Before we can look at your open-source contributions, you (or your employer or company depending on the situation) will need to sign a Contributor License Agreement (CLA). You can sign it here.

Once the CLA will be signed, please reply here and tell us. For example: I signed it, My employer signed it or My company signed it.

Thank you for taking the time to contribute!

@Eric2Hamel

This comment has been minimized.

Copy link
Contributor Author

commented Jul 25, 2019

@Eric2Hamel

This comment has been minimized.

Copy link
Contributor Author

commented Jul 25, 2019

@guillaume-chevalier

This comment has been minimized.

Copy link
Member

commented Jul 29, 2019

Thank you! We'll review the PR soon

@guillaume-chevalier
Copy link
Member

left a comment

Good code in general, thank you! I have a few suggestions that would improve the clarity of the code upon being fixed.

neuraxle/steps/numpy.py Outdated Show resolved Hide resolved
neuraxle/metaopt/random.py Outdated Show resolved Hide resolved
neuraxle/metaopt/random.py Outdated Show resolved Hide resolved
neuraxle/metaopt/random.py Show resolved Hide resolved
neuraxle/steps/numpy.py Outdated Show resolved Hide resolved
testing/metaopt/test_random.py Outdated Show resolved Hide resolved
testing/metaopt/test_random.py Outdated Show resolved Hide resolved
testing/metaopt/test_random.py Outdated Show resolved Hide resolved
testing/metaopt/test_random.py Outdated Show resolved Hide resolved
neuraxle/metaopt/random.py Show resolved Hide resolved

Eric2Hamel added some commits Aug 4, 2019

- WalkForwardTimeSeriesCrossValidation to reflect change discuss in P…
…R change request.

- Add a AnchoredWalkForwardTimeSeriesCrossValidation
- Add unittest for AnchoredWalkForwardTimeSeriesCrossValidation
- Make more clear the magic number by encapsulating it in parameter.
- Make multiple run of the same test with a parametrize pytest fixture function both with AnchoredWalkForwardTimeSeriesCrossValidation and WalkForwardTimeSeriesCrossValidation.
- Add a NumpyConcatenateOnCustomAxis
- Remove the NumpyConcatenateOnTimeSeries.
- Write more message in the docString of AnchoredWalkForwardTimeSeriesCrossValidation and WalkForwardTimeSeriesCrossValidation.
- Add docstring to NumpyConcatenateOnCustomAxis, NumpyConcatenateInne…
…rFeatures, NumpyConcatenateOuterBatch.

- Modify the `window_size` attribute to `validation_window_size` of AnchoredWalkForwardTimeSeriesCrossValidation.
- Modify the docstring.

@cla-bot cla-bot bot added the cla-signed label Aug 4, 2019

@Eric2Hamel

This comment has been minimized.

Copy link
Contributor Author

commented Aug 4, 2019

@guillaume-chevalier the modification has been made, could you review the change see if it is okay.

@guillaume-chevalier
Copy link
Member

left a comment

Things seems good already. I only have a few minor comments that would be nice to fix before merging, or to be fixed in another PR.

neuraxle/metaopt/random.py Show resolved Hide resolved
neuraxle/metaopt/random.py Outdated Show resolved Hide resolved
neuraxle/metaopt/random.py Show resolved Hide resolved
neuraxle/metaopt/random.py Outdated Show resolved Hide resolved
neuraxle/metaopt/random.py Outdated Show resolved Hide resolved
neuraxle/metaopt/random.py Outdated Show resolved Hide resolved
neuraxle/steps/numpy.py Show resolved Hide resolved
neuraxle/steps/numpy.py Outdated Show resolved Hide resolved
neuraxle/steps/numpy.py Outdated Show resolved Hide resolved
testing/metaopt/test_random.py Show resolved Hide resolved

Eric2Hamel added some commits Aug 17, 2019

- Inherit WalkForwardTimeSeriesCrossValidation from AnchoredWalkForwa…
…rdTimeSeriesCrossValidation to same duplicate code

- Switch position of training_window_size and validation_window_size
- Add unittest for the switch position of training_window_size and validation_window_size
- Move some docstring comment in the constructor instead of the class itself.
- Change some docstring comment for NumpyConcatenateInnerFeatures, NumpyConcatenateOnCustomAxis, NumpyConcatenateOuterBatch
- Change some docstring for WalkForwardTimeSeriesCrossValidation and AnchoredWalkForwardTimeSeriesCrossValidation.
@guillaume-chevalier
Copy link
Member

left a comment

Looks good, thanks!

@guillaume-chevalier guillaume-chevalier merged commit 79a023c into Neuraxio:0.1 Aug 18, 2019

1 check passed

verification/cla-signed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.