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 DF upsample #163

Merged
merged 7 commits into from
Feb 27, 2024
Merged

Adding DF upsample #163

merged 7 commits into from
Feb 27, 2024

Conversation

Bidek56
Copy link
Collaborator

@Bidek56 Bidek56 commented Feb 10, 2024

Adding DF upsample to close #162

Copy link
Collaborator

@universalmind303 universalmind303 left a comment

Choose a reason for hiding this comment

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

just 1 small suggestion. Overall looks good.

Bidek56 and others added 2 commits February 11, 2024 13:19
* ... })
* > df.drop(['ham', 'apple'])
* ... });
* > console.log(df.drop(['ham', 'apple']).toString());
Copy link
Collaborator

Choose a reason for hiding this comment

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

you shouldn't need toString() here. the node.util.inspect already properly displays the dataframe in console.log

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

but It does not work in Bun b/c I get ProxyObject without .toString():

ProxyObject {
  foo: shape: (3,)
Series: 'foo' [f64]
[
	1.0
	2.0
	3.0
],
  bar: shape: (3,)
Series: 'bar' [f64]
[
	6.0
	7.0
	8.0
],
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

if that's the case, that'd be a bug in bun. The toString should be unnecessary.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I pinged Jarred about this issue. Thx

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I am not sure if it's a bun issues, b/c this works fine in bun:

console.log(
   pl.DataFrame([{ a: 1, b: 2,}])[Symbol.for("nodejs.util.inspect.custom")]()
);

It results in:

shape: (1, 2)
┌─────┬─────┐
│ a   ┆ b   │
│ --- ┆ --- │
│ f64 ┆ f64 │
╞═════╪═════╡
│ 1.0 ┆ 2.0 │
└─────┴─────┘

Copy link
Collaborator

Choose a reason for hiding this comment

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

yeah this is definitely an issue with bun. If they claim to be 100% interopable with node.js, then the outputs should be the same. I'd suggest opening an issue with them.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Node and Bun output for this line is the same:

console.log(
   pl.DataFrame([{ a: 1, b: 2,}])[Symbol.for("nodejs.util.inspect.custom")]()
);

You had an issue open with Bun but closed it. Any reason?

Copy link
Collaborator

Choose a reason for hiding this comment

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

yes the symbol works as expected, but console.logging without it does not.

console.log(pl.DataFrame({a: [1]}))

this should be identical in both. If it's not, then that is a bug in bun.

Copy link
Collaborator Author

@Bidek56 Bidek56 Feb 16, 2024

Choose a reason for hiding this comment

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

Hopping that this PR from the Bun team will fix it.

@universalmind303 universalmind303 merged commit 8ecf9bd into pola-rs:main Feb 27, 2024
9 checks passed
@Bidek56 Bidek56 deleted the issue-162 branch March 26, 2024 13:12
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.

Add DataFrame.upsample
2 participants