-
Notifications
You must be signed in to change notification settings - Fork 1
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
Feature/flask server render #20
base: master
Are you sure you want to change the base?
Conversation
This makes it consistent everywhere.
This automates the detections of sensor channels in the logfile.
…EC equals exactly zero.
… is being filled.
… work for a specific use case
Feature/insitu log parsing
@@ -0,0 +1,141 @@ | |||
import matplotlib |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this image server is probably best served from another repository rather than inside the analytics library, because this will introduce flask dependencies that really don't have anything to do with analyzing data.
We can make a branch of the https://github.com/platypusllc/cloud private repository where we can put this image server, it uses platypus/analytics as a dependency so you can still import platypus.io.logs
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This generally looks good, the only major things I would suggest are:
- moving the image server over to a branch of the
platypus/cloud
repository instead of here - removing the IPython dependencies from
setup.py
(since they probably aren't sufficient anyway and make any headless processing needlessly heavier) and instead put them into a setup guide in thenotebooks/README.md
which is already pretty good.
'jupyter', | ||
'ipyleaflet', | ||
'sklearn', | ||
'flask' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are there any other packages besides flask that we could drop if we remove the webserver, or are all of the rest of these required for this package?
'scipy', | ||
'utm', | ||
'matplotlib', | ||
'jupyter', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think jupyter is a strict requirement here, since it is only used for the notebooks. It might make sense to make this an extras_require
(http://peak.telecommunity.com/DevCenter/setuptools#declaring-extras-optional-features-with-their-own-dependencies) or omit it entirely here and put it into the README.md
for the notebooks. Having this here will make it hard to use this package for headless data packaging.
'utm', | ||
'matplotlib', | ||
'jupyter', | ||
'ipyleaflet', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can ipyleaflet
be installed this way? I thought you needed to also run something like the following:
$ pip install ipyleaflet
$ jupyter nbextension enable --py --sys-prefix ipyleaflet # can be skipped for
notebook 5.3 and above
@@ -1,4 +1,3 @@ | |||
#!/usr/bin/env python | |||
""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for cleaning this up 👍
Added basic data server (ERM and Colombia using now) that runs on Linux (AWS instance at http://ec2-52-14-194-26.us-east-2.compute.amazonaws.com:5000/ can be seen)