Feature/do discovery #949

alrocar · 2019-08-30T06:54:44Z

Closes #960

cartoframes/data/catalog/dataset.py

cartoframes/data/catalog/country.py

cartoframes/data/catalog/dataset.py

test/data/catalog/repository/test_variable_repo.py

test/data/catalog/repository/test_geography_repo.py

test/data/catalog/repository/test_dataset_repo.py

test/data/catalog/repository/test_country_repo.py

test/data/catalog/repository/test_category_repo.py

test/data/catalog/test_do.py

test/data/catalog/test_dataset.py

test/data/catalog/test_country.py

test/data/catalog/repository/test_category_repo.py

cartoframes/data/catalog/dataset.py

do_metadata_key.py

test/data/catalog/test_variable.py

test/data/catalog/test_geography.py

test/data/catalog/test_category.py

alrocar · 2019-09-09T16:31:42Z

I see tests are in red (also we already merged the namespaces refactor to the develop branch). Could we try to merge develop + make the tests pass before reviewing? 🙏 I think that would be easier.

Thanks!

cartoframes/data/catalog/__init__.py

esloho · 2019-09-10T10:32:53Z

Tests are green, weeeeee!!

houndci-bot · 2019-09-10T13:31:51Z

test/data/observatory/test_variable.py

+try:
+    from unittest.mock import Mock, patch
+except ImportError:
+    from mock import Mock, patch


'mock.Mock' imported but unused

test/data/observatory/test_geography.py

houndci-bot · 2019-09-10T13:31:51Z

test/data/observatory/test_dataset.py

+try:
+    from unittest.mock import Mock, patch
+except ImportError:
+    from mock import Mock, patch


'mock.Mock' imported but unused

houndci-bot · 2019-09-10T13:31:51Z

test/data/observatory/test_country.py

+try:
+    from unittest.mock import Mock, patch
+except ImportError:
+    from mock import Mock, patch


'mock.Mock' imported but unused

houndci-bot · 2019-09-10T13:31:52Z

test/data/observatory/test_category.py

+try:
+    from unittest.mock import Mock, patch
+except ImportError:
+    from mock import Mock, patch


'mock.Mock' imported but unused

houndci-bot · 2019-09-10T13:31:52Z

test/data/observatory/test_catalog.py

+try:
+    from unittest.mock import Mock, patch
+except ImportError:
+    from mock import Mock, patch


'mock.Mock' imported but unused

cartoframes/data/observatory/repository/category_repo.py

cartoframes/data/observatory/catalog.py

esloho · 2019-09-10T13:42:11Z

cartoframes/data/observatory/category.py

+import pandas as pd
+
+from cartoframes.data.observatory.repository.category_repo import get_category_repo
+from cartoframes.data.observatory.repository.dataset_repo import get_dataset_repo


Since we want the API methods to be static we can't have the repositories instances in these classes, that's why I created those helper methods, but I'm not sure if I like it xD

Would be better to import the Repo instead and create a private instance for the module? Something like:

from cartoframes.data.catalog.repository.category_repo import CategoryRepository __category_repo = CategoryRepository()

Since we are going to use the Repository pattern to switch to a different backend API when needed, what of these options would be easier to replace when we have a different backend API?

The wrapper functions

Instantiate the repository class

I prefer the helper. Less code and less coupling.

what of these options would be easier to replace when we have a different backend API?

I think, we could change to a different backend just changing the RepoClient.

cartoframes/data/observatory/repository/category_repo.py

cartoframes/data/observatory/repository/repo_client.py

alrocar

Upss turns out I'm the author of this pull request, so I can just comment.

👏 Great work so far!

I added a bunch of comments mostly of aesthetic stuff. I'd do this:

Let's try to change to relative paths to follow our library convention.
The rest of my comments, don't need to be tackled in this PR, otherwise it's going to be a never ending task.

I'd merge this one (after the relative paths thing being changed) so we can start integrating with the enrichment functions and create a new issue with some of my suggestions (the ones we decide to tackle).

cartoframes/data/observatory/catalog.py

alrocar · 2019-09-10T16:26:08Z

cartoframes/data/observatory/category.py

+import pandas as pd
+
+from cartoframes.data.observatory.repository.category_repo import get_category_repo
+from cartoframes.data.observatory.repository.dataset_repo import get_dataset_repo


Since we are going to use the Repository pattern to switch to a different backend API when needed, what of these options would be easier to replace when we have a different backend API?

The wrapper functions

Instantiate the repository class

cartoframes/data/observatory/repository/category_repo.py

alrocar · 2019-09-10T16:38:57Z

cartoframes/data/observatory/repository/category_repo.py

+    def _to_categories(results):
+        from cartoframes.data.observatory.category import Categories
+
+        return Categories([CategoryRepository._to_category(result) for result in results])


Related to my previous comment, I see we have two ways of creating a Category instance, am I right?

CategoryRepository._to_category(result) or using the constructor.

If that's true, I'd try to standardize that.

This comment applies to the rest of repositories.

cartoframes/data/observatory/repository/repo_client.py

cartoframes/data/observatory/repository/category_repo.py

alrocar · 2019-09-10T16:51:18Z

discovery API.ipynb

@@ -0,0 +1,1171 @@
+{


Since Github does not deal well with changes in notebook files, I think it's better to commit the notebooks without the outputs. So let's remove them, please.

OK, we added it here only for sharing and feedback gathering but I guess there's no need of that anymore?

Yep, that's useful. Let's remove them then before merging.

alrocar · 2019-09-10T16:55:06Z

cartoframes/data/observatory/geography.py

+_GEOGRAPHY_FIELD_ID = 'id'
+
+
+class Geography(pd.Series):


I'm seeing in the data model, that Geography has a geom_coverage column of type GEOMETRY. How are we dealing with this type of column?

We're not, panda is dealing with it. We don't deal with columns anymore. Does it sound right?

👍 It's not important right now, but let's take into account in the future, since if we want to do some spatial operation, we'd need to map Geography as a geodataframe instead of a dataframe.

oleurud

A first comment about singleton

cartoframes/data/observatory/repository/repo_client.py

oleurud

It looks cool :) I have added some comments and one more here: maybe we could add a parent class of the models and another one of the repositories, avoiding repeat some methods (JAVA vision :) ), but probably I would not do it now in this PR

oleurud · 2019-09-11T07:16:18Z

cartoframes/data/observatory/category.py

+import pandas as pd
+
+from cartoframes.data.observatory.repository.category_repo import get_category_repo
+from cartoframes.data.observatory.repository.dataset_repo import get_dataset_repo


I prefer the helper. Less code and less coupling.

what of these options would be easier to replace when we have a different backend API?

I think, we could change to a different backend just changing the RepoClient.

cartoframes/data/observatory/repository/category_repo.py

oleurud · 2019-09-11T07:43:35Z

cartoframes/data/observatory/repository/category_repo.py

+        return Category({
+            'id': result['id'],
+            'name': result['name']
+        })


Mapping the classes, we could have a problem with previous versions in the future: if we release a version doing the mapping, and after that, we change a column from the database, the released version will fail.

As we want to have all the catalog public, we could put the responsibility of the API properties in the backend and making the CARTOframes client transparent

oleurud · 2019-09-11T07:45:52Z

cartoframes/data/observatory/repository/country_repo.py

+
+    @staticmethod
+    def _to_country(result):
+        from cartoframes.data.observatory.country import Country


I prefer doing the import on the top of the file. Are you trying to avoid the circular pain?

Yes, see my comment above. This is the only way I found to make it work without changing the API. I think it's a design flaw due to mixing model and behavior :(

houndci-bot · 2019-09-11T09:17:30Z

test/data/observatory/repository/test_category_repo.py

+class TestCategoryRepo(unittest.TestCase):
+
+    def setUp(self):
+         RepoClient.get_categories = Mock(return_value=[db_category1, db_category2])


indentation is not a multiple of four

alrocar

🚀 LGTM!

I cannot approve the PR because I created it, but it's approved.

Let's clean the notebook output before merging, please.

Esther Lozano and others added 4 commits August 28, 2019 19:08

Add preliminary classes for DO discovery

fee3244

discovery API

97eccbb

Add examples to DO notebook

83a6061

detail about describe

1036373

houndci-bot reviewed Aug 30, 2019

View reviewed changes

cartoframes/data/catalog/dataset.py Outdated Show resolved Hide resolved

cartoframes/data/catalog/country.py Outdated Show resolved Hide resolved

esloho added 2 commits August 30, 2019 19:53

First draft of models and repository logic

6775e26

Add models for lists of entities

7b58233

houndci-bot reviewed Sep 2, 2019

View reviewed changes

cartoframes/data/catalog/dataset.py Outdated Show resolved Hide resolved

Add unitary tests for repo classes

fae55d6

houndci-bot reviewed Sep 3, 2019

View reviewed changes

esloho added 3 commits September 3, 2019 16:22

Fix blank lines

7b9c02d

Make entities an extension of pandas dataframes

3aef565

Fix panda classes extension

5ff2b1a

houndci-bot reviewed Sep 4, 2019

View reviewed changes

Fix return type when accessing model properties

13d95d1

houndci-bot reviewed Sep 4, 2019

View reviewed changes

cartoframes/data/catalog/dataset.py Outdated Show resolved Hide resolved

Add key file for easy change

94ed7e3

houndci-bot reviewed Sep 5, 2019

View reviewed changes

do_metadata_key.py Outdated Show resolved Hide resolved

Add missing navigation levels for all models

18d254a

houndci-bot reviewed Sep 6, 2019

View reviewed changes

test/data/catalog/test_variable.py Outdated Show resolved Hide resolved

test/data/catalog/test_geography.py Outdated Show resolved Hide resolved

test/data/catalog/test_category.py Outdated Show resolved Hide resolved

esloho added 2 commits September 9, 2019 12:29

Add disclaimer

607c623

Merge branch 'develop' into feature/do-discovery

f3c5d2a

alrocar added the discovery label Sep 9, 2019

Fix DO test

b445349

houndci-bot reviewed Sep 9, 2019

View reviewed changes

cartoframes/data/catalog/__init__.py Outdated Show resolved Hide resolved

esloho added 4 commits September 9, 2019 19:11

Remove init content until confirmation

c18a9ce

Use absolut import instead of relative

78318cf

Merge branch 'develop' into feature/do-discovery

636e48e

Update namespaces from develop

23878c5

esloho added 3 commits September 10, 2019 12:49

Fix singleton

f4da0bf

Undo unwanted change added by error

5fdd5a6

Rename namespaces as agreed

57015a6

houndci-bot reviewed Sep 10, 2019

View reviewed changes

esloho reviewed Sep 10, 2019

View reviewed changes

cartoframes/data/observatory/repository/category_repo.py Show resolved Hide resolved

cartoframes/data/observatory/catalog.py Show resolved Hide resolved

esloho reviewed Sep 10, 2019

View reviewed changes

alrocar commented Sep 10, 2019

View reviewed changes

alrocar assigned esloho Sep 11, 2019

oleurud reviewed Sep 11, 2019

View reviewed changes

cartoframes/data/observatory/repository/repo_client.py Show resolved Hide resolved

oleurud reviewed Sep 11, 2019

View reviewed changes

esloho added 2 commits September 11, 2019 10:47

Use relative namespaces instead of absolute ones

9d2f167

Stop mapping model in API

9758353

houndci-bot reviewed Sep 11, 2019

View reviewed changes

alrocar mentioned this pull request Sep 11, 2019

Data Discovery - properties #961

Closed

alrocar commented Sep 11, 2019

View reviewed changes

Remove notebook

47bb3e4

esloho merged commit e3515b3 into develop Sep 11, 2019

Jesus89 deleted the feature/do-discovery branch September 30, 2019 16:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/do discovery #949

Feature/do discovery #949

alrocar commented Aug 30, 2019 •

edited

Loading

alrocar commented Sep 9, 2019

esloho commented Sep 10, 2019

houndci-bot Sep 10, 2019

houndci-bot Sep 10, 2019

houndci-bot Sep 10, 2019

houndci-bot Sep 10, 2019

houndci-bot Sep 10, 2019

esloho Sep 10, 2019

alrocar Sep 10, 2019 •

edited

Loading

oleurud Sep 11, 2019

alrocar left a comment

alrocar Sep 10, 2019 •

edited

Loading

alrocar Sep 10, 2019

alrocar Sep 10, 2019

esloho Sep 11, 2019

alrocar Sep 11, 2019

alrocar Sep 10, 2019

esloho Sep 11, 2019

alrocar Sep 11, 2019

oleurud left a comment

oleurud left a comment

oleurud Sep 11, 2019

oleurud Sep 11, 2019

oleurud Sep 11, 2019

esloho Sep 11, 2019 •

edited

Loading

houndci-bot Sep 11, 2019

alrocar left a comment

Feature/do discovery #949

Feature/do discovery #949

Conversation

alrocar commented Aug 30, 2019 • edited Loading

alrocar commented Sep 9, 2019

esloho commented Sep 10, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alrocar Sep 10, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alrocar left a comment

Choose a reason for hiding this comment

alrocar Sep 10, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oleurud left a comment

Choose a reason for hiding this comment

oleurud left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

esloho Sep 11, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alrocar left a comment

Choose a reason for hiding this comment

alrocar commented Aug 30, 2019 •

edited

Loading

alrocar Sep 10, 2019 •

edited

Loading

alrocar Sep 10, 2019 •

edited

Loading

esloho Sep 11, 2019 •

edited

Loading