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

Feature/962 pandas integration #1014

Merged
merged 9 commits into from
Sep 23, 2019
Merged

Conversation

esloho
Copy link
Contributor

@esloho esloho commented Sep 20, 2019

Closes #962

Regarding the second problem exposed in the issue #962 we had to compromised: we'll raise an exception when trying to access with a column Series any method that requires a row Series.

When asking for all the datasets by a particular category (Category.datasets()), that Category instance must represent a single entity like when using loc/iloc in a DataFrame or when using the get_by_id method. But if it represents a column like when accessing a property of the DataFrame (Categories['name']) there's not a single id we can use to search the associated datasets. Ideally these cases shouldn't be represented as Category instances, but since we couldn't find a way to avoid it let's try this solution.

This PR also includes a missing change in last PR: return None (instead of empty list) when no entities are found in the server.

@esloho
Copy link
Contributor Author

esloho commented Sep 20, 2019

Maybe @oleurud would like to CR this one.

@oleurud oleurud self-assigned this Sep 20, 2019
Copy link
Contributor

@oleurud oleurud left a comment

Choose a reason for hiding this comment

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

About the message in the DiscoveryException, I am not sure at all, I think a user is not going to understand the message.

cartoframes/data/observatory/category.py Outdated Show resolved Hide resolved
@esloho
Copy link
Contributor Author

esloho commented Sep 20, 2019

About the message in the DiscoveryException, I am not sure at all, I think a user is not going to understand the message.

I tried different versions and this was the best one, so if you have any suggestion I'd be in debt with you forever and ever ( @alrocar @oleurud guiño guiño)

Copy link
Contributor

@oleurud oleurud left a comment

Choose a reason for hiding this comment

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

Let's go!

A comment for future PRs: I think we can extract some common methods of the entities to a parent class making the code easy to maintain

@esloho
Copy link
Contributor Author

esloho commented Sep 23, 2019

A comment for future PRs: I think we can extract some common methods of the entities to a parent class making the code easy to maintain

Agree. I tried at first but there wasn't much that could be extracted. I'll give it another look now that we have more methods :) Thanks!

@oleurud
Copy link
Contributor

oleurud commented Sep 23, 2019

Acceptance 🍏

@oleurud oleurud merged commit affad11 into develop Sep 23, 2019
@oleurud oleurud deleted the feature/962-pandas-integration branch September 23, 2019 14:10
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.

Better integration of the catalog with pandas
3 participants