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

Switch from Golang to Python #43

Merged
merged 15 commits into from
Apr 4, 2021
Merged

Conversation

jthomperoo
Copy link
Owner

Resolves #38

@codecov-io
Copy link

codecov-io commented Oct 31, 2020

Codecov Report

Merging #43 (f3c71d0) into master (7911fe6) will decrease coverage by 0.52%.
The diff coverage is 86.76%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #43      +/-   ##
==========================================
- Coverage   75.79%   75.26%   -0.53%     
==========================================
  Files           9       11       +2     
  Lines         252      283      +31     
==========================================
+ Hits          191      213      +22     
- Misses         60       65       +5     
- Partials        1        5       +4     
Flag Coverage Δ
unittests 75.26% <86.76%> (-0.53%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
internal/config/config.go 100.00% <ø> (ø)
internal/evaluate/evaluate.go 100.00% <ø> (ø)
internal/fake/algorithm.go 0.00% <0.00%> (ø)
internal/fake/predicter.go 100.00% <ø> (ø)
internal/fake/stored.go 100.00% <ø> (ø)
internal/prediction/prediction.go 100.00% <ø> (ø)
internal/stored/stored.go 0.00% <ø> (ø)
internal/prediction/linear/linear.go 85.71% <73.33%> (ø)
internal/prediction/holtwinters/holtwinters.go 91.54% <86.66%> (ø)
internal/algorithm/algorithm.go 100.00% <100.00%> (ø)
... and 4 more

@kevjin
Copy link

kevjin commented Mar 30, 2021

hey @jthomperoo! I just saw this, and I'm curious what other TODOs do you need to get this merged in? or is it just like final/edge case testing? thanks!

@jthomperoo
Copy link
Owner Author

Hey @kevjin - thanks for your other pull request btw, just having a look through now and I'll get it merged in - for this work to be honest I had left it as I didn't have much time. As far as I can remember it was 90% done, just wanted to test that it worked without breaking anything. Thanks for reminding me of this, I think I'll have a look at these changes at the weekend and see if I can get it merged in. Feel free to have a look through the changes.

There was a request to add ARIMA forecasting to this (#37) which was on the to-do list after this Python stuff was added in, which would hopefully be easy enough with the Python libraries (easier than reimplementing in Golang anyway!).

@kevjin
Copy link

kevjin commented Mar 30, 2021

awesome thanks! I'm actually trying to get this working for my own LSTM model haha, so I'll definitely be looking, and doing some of my own manual testing on this PR. I'll let you know if I find any bugs!


const (
entrypoint = "python"
shellTimeout = 10000
Copy link

Choose a reason for hiding this comment

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

imo a hard 10 second timeout is a little too restrictive. Some models, like ARIMA, will probably take longer to run. Also the more storedValues you have, the longer it'll take the model to run. What about 30 seconds?

Although, I'm not sure if the CPA would respect the longer timeout (haven't really looked at it)

Copy link
Owner Author

Choose a reason for hiding this comment

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

Yep good point, I've made a bunch of changes so that this value is configurable, and upped the default timeout to 30s.

@jthomperoo jthomperoo merged commit bd54dba into master Apr 4, 2021
@jthomperoo jthomperoo deleted the switch_from_golang_to_python branch April 4, 2021 16:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Investigate using Python for statistical algorithms
3 participants