Skip to content

Implemented sqrt for cmath #3047

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

Merged
merged 3 commits into from
Sep 13, 2021
Merged

Implemented sqrt for cmath #3047

merged 3 commits into from
Sep 13, 2021

Conversation

Codemonk-adi
Copy link
Contributor

This is my first time working with rust, pls tell me if my code is wrong, i ran the test but couldnt unserstand the output

Copy link
Member

@youknowone youknowone left a comment

Choose a reason for hiding this comment

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

Thank you for contributing! The added code looks great but I have a few comments.

  1. Please do not add Cargo.lock changes here. I have no idea why it is changed without any Cargo.toml changes but I think this is not what you intended.
  2. Please run rustfmt. cargo fmt --all is usually enough.
  3. You may forgot to correctly set user.name and user.email. Search for git config user.email how to do it and revise your commit metadata by git commit --amend --reset-author

@fanninpm
Copy link
Contributor

i ran the test but couldnt unserstand the output

I don't believe we're currently at the point where we can confidently test your code just by running the CPython test suite (see these lines of test_cmath.py). Perhaps, in the interim, we could put some temporary tests in the extra_tests/snippets/ directory.

@Codemonk-adi Codemonk-adi marked this pull request as draft September 13, 2021 17:01
@youknowone
Copy link
Member

@fanninpm In this case, I think we'd better to edit the test file to run actual test rather than adding our own test. The running is suppressed but there is already tests

@Codemonk-adi Codemonk-adi marked this pull request as ready for review September 13, 2021 17:02
@youknowone
Copy link
Member

a tip for new open source contributors: never use git merge or git pull. we only need git fetch and git rebase. (or git pull --rebase, but too easy to make mistake)

@DimitrisJim
Copy link
Member

Fixed up the tests a bit to allow sqrt to run, I'll take care of the other PRs too and (when they all hopefully finish) bring test_cmath to its original form.

@hk1997
Copy link

hk1997 commented Sep 13, 2021

Fixed up the tests a bit to allow sqrt to run, I'll take care of the other PRs too and (when they all hopefully finish) bring test_cmath to its original form.

Hi @DimitrisJim , I'm about to raise a PR for acos and acosh implementation, were these comments in the test file suggested by you to @Codemonk-adi ?

Should I wait for his PR to be merged?

@DimitrisJim
Copy link
Member

Should I wait for his PR to be merged?

Yup. Lets wait and see how we'll be running the tests. I'm personally open to either having a new variable or doing it as I've currently done. Since I'll try and follow most PRs for cmath.rs I think I can keep track of things whatever the case.

@youknowone opinions here?

@youknowone
Copy link
Member

My suggestion is replcing line 331 like below:

            complex_fn = getattr(cmath, fn, None)
            if complex_fn is None:
                continue

Then newly added function always will be tested but no error will be raised for not added one.
We can finally remove it when every functions are finally added.

@DimitrisJim
Copy link
Member

Yup, that was a better idea. (Ref #3039 to keep track of the PRs.)

Copy link
Member

@DimitrisJim DimitrisJim left a comment

Choose a reason for hiding this comment

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

Looks good to me, thanks!

@youknowone
Copy link
Member

@Codemonk-adi user.email doesn't look correctly set

@Codemonk-adi
Copy link
Contributor Author

@youknowone
I did git config --global user.name "Codemonk-adi"
and then pushed, can you pls tell me what should I do?

@Snowapril
Copy link
Contributor

@Codemonk-adi You should also set git config --global user.email "your email" then problems will be disappeared 😊

@youknowone youknowone mentioned this pull request Sep 14, 2021
16 tasks
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.

6 participants