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.

Sign up for GitHub

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

Jump to bottom

31 remove tests #32

Merged
merged 7 commits into from
Dec 9, 2023
Merged

31 remove tests #32

merged 7 commits into from
Dec 9, 2023

Conversation

tschm
Copy link
Collaborator

@tschm tschm commented Dec 9, 2023

No description provided.

@tschm tschm linked an issue Dec 9, 2023 that may be closed by this pull request
Remove tests #31
Closed
@tschm tschm requested review from phschiele and kasperjo December 9, 2023 13:55
@tschm
Copy link
Collaborator Author

tschm commented Dec 9, 2023

jinja2 is needed for the table produced at the end of the experiments

@tschm
Copy link
Collaborator Author

tschm commented Dec 9, 2023

I have removed the sys.path hack... :-)

@tschm
Copy link
Collaborator Author

tschm commented Dec 9, 2023

I have removed the entire tests folder. Not needed

@tschm
Copy link
Collaborator Author

tschm commented Dec 9, 2023

I know run 'make experiments during ci/cd'

@tschm
Copy link
Collaborator Author

tschm commented Dec 9, 2023

I made sure there is a checkpoints folder :-)

@tschm
Copy link
Collaborator Author

tschm commented Dec 9, 2023

And I solved the Secret Santa problem ;-)

@phschiele
Copy link
Collaborator

@tschm I only added the hack because I wanted the main file that holds the actual reference implementation to be in the root of the repo (markowitz.py). I'm not sure if moving it so a subfolder is preferred here.

Are you sure we need any of these additional libraries?

@tschm
Copy link
Collaborator Author

tschm commented Dec 9, 2023

We need at least jinja2 for now. My experiments crashed when it came to creating the table saying jinja2 is missing. Loguru is not needed yet. Cvxsimulator also not needed at this stage

@tschm
Copy link
Collaborator Author

tschm commented Dec 9, 2023

For me experiments.py is the entry point and hence could be top level. I think markowitz.py is better in the experiments "package". Easier and cleaner?

phschiele
phschiele reviewed Dec 9, 2023
View reviewed changes
experiments/backtest.py Outdated Show resolved Hide resolved
phschiele
phschiele reviewed Dec 9, 2023
View reviewed changes
requirements.txt Outdated Show resolved Hide resolved
phschiele
phschiele approved these changes Dec 9, 2023
View reviewed changes
Copy link
Collaborator

@phschiele phschiele left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Had two small comments, then good to go from my side.

tschm and others added 2 commits December 9, 2023 21:36
@tschm @phschiele
Update requirements.txt
4cd1e48 
Co-authored-by: Philipp Schiele <44360364+phschiele@users.noreply.github.com>
@tschm
Update backtest.py
05665a7 
Remove the hack traces
@tschm
Copy link
Collaborator Author

tschm commented Dec 9, 2023 via email

@tschm tschm merged commit 2b65447 into main Dec 9, 2023
2 checks passed
@tschm tschm deleted the 31-remove-tests branch December 9, 2023 17:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@phschiele phschiele phschiele approved these changes

@kasperjo kasperjo Awaiting requested review from kasperjo

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Remove tests
2 participants
@tschm @phschiele