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

initialize nestedframe #5

Merged
merged 2 commits into from
Apr 4, 2024
Merged

initialize nestedframe #5

merged 2 commits into from
Apr 4, 2024

Conversation

dougbrn
Copy link
Collaborator

@dougbrn dougbrn commented Apr 3, 2024

Initializes the NestedFrame module. Adds a few properties and an initial add_nested function.

@dougbrn dougbrn requested a review from hombit April 3, 2024 19:50
Copy link

github-actions bot commented Apr 3, 2024

Before [05cc195] After [bc01396] Ratio Benchmark (Parameter)
2.30±1s 1.76±1s ~0.77 benchmarks.time_computation
1.54k 2.35k 1.53 benchmarks.mem_list

Click here to view all benchmarks.

Copy link
Collaborator

@hombit hombit left a comment

Choose a reason for hiding this comment

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

Looks great! Please see some suggestions

@@ -1,4 +1,5 @@
from .example_module import greetings, meaning
from .nestedframe import NestedFrame # noqa
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's remove noqa and add to __all__?

@@ -0,0 +1 @@
from .core import * # noqa
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we really need star-import here? It would add pd and other stuff we don't really need

"""returns a dictionary of columns for each base/nested dataframe"""
all_columns = {"base": self.columns}
for column in self.columns:
if hasattr(self[column], "nest"):
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would check column's dtype instead, it should be NestedDtype. No code suggestion here, because we need to import it first

from nested_pandas import NestedDtype

if isinstance(self[column].dtype, NestedDtype):

all_columns = {"base": self.columns}
for column in self.columns:
if hasattr(self[column], "nest"):
nest_cols = self[column].iloc[0].columns # TODO: Improve access to columns
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
nest_cols = self[column].iloc[0].columns # TODO: Improve access to columns
nest_cols = self[column].nest.fields()

"""retrieves the base column names for all nested dataframes"""
nest_cols = []
for column in self.columns:
if hasattr(self[column], "nest"):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
if hasattr(self[column], "nest"):
if isinstance(self[column].dtype, NestedDtype):

Comment on lines 51 to 53
else:
return False
else:
Copy link
Collaborator

Choose a reason for hiding this comment

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

It is a matter of taste, but I believe we don't need these else:s here

@dougbrn dougbrn requested a review from hombit April 3, 2024 20:25
@dougbrn
Copy link
Collaborator Author

dougbrn commented Apr 3, 2024

Thanks for the comments, I've addressed them all in the latest commit

Copy link
Collaborator

@hombit hombit left a comment

Choose a reason for hiding this comment

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

Looks great

@dougbrn dougbrn merged commit e04f558 into main Apr 4, 2024
8 checks passed
@dougbrn dougbrn deleted the init_nestedframe branch April 4, 2024 20:03
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