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

Adding dynamic routing and weather service #176

Merged
merged 1 commit into from
May 11, 2017

Conversation

saijel
Copy link
Contributor

@saijel saijel commented May 8, 2017

Creating new PR that was based off of #169.

List of changes:

  • added structure to allow dynamic routing
  • added weather microservice
  • added 2 new workflows to Config.py

@saijel saijel requested a review from jhauswald May 8, 2017 19:25
@saijel saijel changed the title adding dynamic routing and weather service Adding dynamic routing and weather service May 8, 2017
This was referenced May 11, 2017
lucida/Makefile Outdated
@@ -1,3 +1,3 @@
SUBDIRS=commandcenter imagematching questionanswering calendar speechrecognition djinntonic
SUBDIRS=commandcenter imagematching questionanswering calendar speechrecognition djinntonic weather
Copy link
Contributor

Choose a reason for hiding this comment

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

remove speechrecognition so its not built by default

Copy link
Contributor

Choose a reason for hiding this comment

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

since chrome is going to be default its painful to build


@abc.abstractmethod
def logic_method(self, response_data, service_graph, dcm_node):
"""Decision making logic"""
Copy link
Contributor

Choose a reason for hiding this comment

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

💯

self.create_query_spec('knowledge', [knowledge_input]))
transport.close()

# TODO: I need help trying to make this method look less scary
Copy link
Contributor

Choose a reason for hiding this comment

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

make this comment more descriptive what you would want to do (split into multiple functions, make a class) suggesting next that comes here has an idea.

not_monument = 'cow'

class IMMDCM(Decision):

Copy link
Contributor

Choose a reason for hiding this comment

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

can you throw a doc string here describing a little what's going on. this is a template for DCM nodes so should be well documented.

Copy link
Contributor

Choose a reason for hiding this comment

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

something like:

class IMMDCM(Decision):
"""
bla bla
"""
   def logic_bla

from Decision import*

class WEDCM(Decision):

Copy link
Contributor

Choose a reason for hiding this comment

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

same here on docstring. get into the habit of docstringing.

@@ -0,0 +1,20 @@
What's the weather here?
Copy link
Contributor

Choose a reason for hiding this comment

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

ha really creative training data 🥇

image_file.write(data.c_str(), data.size());
image_file.close();
return image_path;
struct timeval tp;
Copy link
Contributor

Choose a reason for hiding this comment

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

fixes for IMM DB creation right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, the actual addition is on line 119 to convert images before pushing them to DB

// if (pos != std::string::npos) res = img_name.substr(0, pos);
// res = res + ".pb";
// return res;
// string res;
Copy link
Contributor

Choose a reason for hiding this comment

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

what's all this commented out code? maybe not you but if you know put a comment/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no clue why these lines are commented, will delete

// tess->End();
// // TODO: add some preprocessing for a bounding box
//
// tesseract::TessBaseAPI *tess = new tesseract::TessBaseAPI();
Copy link
Contributor

Choose a reason for hiding this comment

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

what's all this commented code? if you don't know why its there lets remove

# Open Weather Map API key
# https://openweathermap.org/api
OWM_API_URL_BASE = 'http://api.openweathermap.org/data/2.5/weather?'
OWM_API_KEY = '362537b891e6df03a316e82565fe4df3' # TODO: remove after PR has been accepted # add your API key here
Copy link
Contributor

Choose a reason for hiding this comment

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

Noted. have your own branch that across all the MRs applies your keys. keep rebasing and applying that branch to master.

@jhauswald jhauswald merged commit 00bf2d2 into claritylab:master May 11, 2017
@saijel saijel deleted the dynamic-routing branch May 11, 2017 19:52
KamalGalrani pushed a commit to KamalGalrani/lucida that referenced this pull request May 12, 2017
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.

2 participants