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 RPC projection class #108

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Conversation

mleotta
Copy link
Collaborator

@mleotta mleotta commented Nov 13, 2017

No description provided.

Copy link
Collaborator

@aashish24 aashish24 left a comment

Choose a reason for hiding this comment

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

code looks reasonable to me 👍 I am wondering if we should just call the model RPCModel instead of RPC_Model?

Also, do we have some test data that we can use to add testing for the model? also perhaps later we can create a general transform model for geospatial.

@mleotta
Copy link
Collaborator Author

mleotta commented Nov 14, 2017

Yes, we can easily make up some test data, but I'm not sure where to put it or how tests are structured in gaia.

@coveralls
Copy link

coveralls commented Nov 14, 2017

Coverage Status

Coverage decreased (-2.7%) to 69.317% when pulling 94c7494 on mleotta:add-rpc into 990173a on OpenDataAnalytics:master.

@aashish24
Copy link
Collaborator

thanks for the changes. The tests are python unit tests.
For one of the example please see here:
(https://github.com/OpenDataAnalytics/gaia/blob/master/tests/cases/test_processors.py). Current the test data resided in tests/data but if we have large enough test data then we can put it on data.kitware.com and have it downloaded during the testing. I can have danlipsa add the testing if you have the test data.

Copy link
Collaborator

@aashish24 aashish24 left a comment

Choose a reason for hiding this comment

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

thanks for fixing the style as well. LGTM if we add the tests later in a separate PR.

@mleotta
Copy link
Collaborator Author

mleotta commented Nov 14, 2017

Yes, can you have Dan help by adding the tests. I won't have time to get to it for a while. I'll send you some data.

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.

3 participants