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

feat: Added ddof arg to std #88

Merged
merged 11 commits into from
May 6, 2024
Merged

Conversation

anopsy
Copy link
Member

@anopsy anopsy commented May 5, 2024

This one is spicy, I'm very curious if I did it right

@anopsy
Copy link
Member Author

anopsy commented May 5, 2024

Okay, I can see what's the problem, I need to check also why it passed the tests

@MarcoGorelli
Copy link
Member

Wow, nice! I'm impressed you figured it all out independently, well done!

I think we need a unit test for this, in which we pass different values of ddof. Fancy trying to add one? Either in test_common.py, or in a new test file

@anopsy
Copy link
Member Author

anopsy commented May 5, 2024

geeeeez, that was tough, I'm still wondering what that "correction: float =1.0" parameter was.
so happy it works

I think we need a unit test for this, in which we pass different values of ddof. Fancy trying to add one? Either in test_common.py, or in a new test file

sure thing!

@MarcoGorelli
Copy link
Member

😆 I'd accidentally copied over correction from the array API, glad you figured out it was wrong

@anopsy
Copy link
Member Author

anopsy commented May 6, 2024

really curious why the pytest-coverage fails, I've run the pytest --cov==narwhals and it didn't complain:
image

Also: I kind of feel that writing the test with nw.all would be more elegant, let me know what do you think

@MarcoGorelli
Copy link
Member

that was fast, cool - you definitely understated your skills 😄 🙌

for the test, I think it's just that your branch is out-of-sync with main - can you run

git fetch upstream
git merge upstream/main
git push origin HEAD

?

You can also remove if df_any is None: return, it's no longer needed (it was just a "trick" I'd introduced to skip running the pyarrow-backed dataframe for CI runs when the pandas version was too old to support them)

@anopsy
Copy link
Member Author

anopsy commented May 6, 2024

image

This is what I get, should I then git pull? (as far as I remember it will ask me about rebasing and I have no idea how to proceed)

thank you, I could definitely improve my git game

@MarcoGorelli
Copy link
Member

Also: I kind of feel that writing the test with nw.all would be more elegant, let me know what do you think

maybe, but what you've done is good, I think it's ok to merge as-is

Copy link
Member

@MarcoGorelli MarcoGorelli left a comment

Choose a reason for hiding this comment

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

nice one @anopsy !

@MarcoGorelli MarcoGorelli merged commit 2392859 into narwhals-dev:main May 6, 2024
13 checks passed
@anopsy anopsy deleted the addargs branch May 6, 2024 10:15
@anopsy anopsy changed the title Added ddof arg to std feat: Added ddof arg to std Jul 27, 2024
@github-actions github-actions bot added the enhancement New feature or request label Jul 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants