The Wayback Machine - https://web.archive.org/web/20201126010812/https://github.com/TheAlgorithms/Python/pull/3934
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

Ohm's Law algorithm added #3934

Merged
merged 17 commits into from Nov 25, 2020
Merged

Ohm's Law algorithm added #3934

merged 17 commits into from Nov 25, 2020

Conversation

@erdum
Copy link
Contributor

@erdum erdum commented Nov 22, 2020

Describe your change:

Finnaly i did it now i hope all checks should be clear

  • Add an algorithm?
  • Fix a bug or typo in an existing algorithm?
  • Documentation change?

Checklist:

  • I have read CONTRIBUTING.md.
  • This pull request is all my own work -- I have not plagiarized.
  • I know that pull requests will not be merged if they fail the automated tests.
  • This PR only changes one algorithm file. To ease review, please open separate PRs for separate algorithms.
  • All new Python files are placed inside an existing directory.
  • All filenames are in all lowercase characters with no spaces or dashes.
  • All functions and variable names follow Python naming conventions.
  • All function parameters and return values are annotated with Python type hints.
  • All functions have doctests that pass the automated testing.
  • All new algorithms have a URL in its comments that points to Wikipedia or other similar explanation.
  • If this pull request resolves one or more open issues then the commit message contains Fixes: #{$ISSUE_NO}.
@erdum

This comment has been minimized.

Copy link
Owner Author

@erdum erdum commented on 3986d64 Nov 22, 2020

I have work a lot on this file and again make new Pr this time i follow all the guidelines styling conventions now lets see what will happen??

erdum added 7 commits Nov 22, 2020
@erdum
Copy link
Contributor Author

@erdum erdum commented Nov 22, 2020

Thank god i made my first ever github contribution, i was very difficult for a beginner, but i did it, jus please review it and merged it i will more happy if you will merged it thanks.

Co-authored-by: xcodz-dot <[email protected]>
@erdum erdum mentioned this pull request Nov 23, 2020
electronics/ohms_law.py Outdated Show resolved Hide resolved
electronics/ohms_law.py Outdated Show resolved Hide resolved
electronics/ohms_law.py Show resolved Hide resolved
electronics/ohms_law.py Outdated Show resolved Hide resolved
electronics/ohms_law.py Outdated Show resolved Hide resolved
erdum and others added 3 commits Nov 24, 2020
Co-authored-by: Christian Clauss <[email protected]>
@erdum
Copy link
Contributor Author

@erdum erdum commented Nov 24, 2020

But cclauss thank you so much showing interest in my code before you nobody even tell me the problems with my code thank you again and i will be add these changes as you tell me .

@erdum
Copy link
Contributor Author

@erdum erdum commented Nov 24, 2020

So i got it what you suggest me is

  1. return dictionary with value name instead value
  2. doctest import is made inside main
  3. Comments should be helpful for reader
  4. And for wrong value input value error should be raised instead returning 0
@dhruvmanila
Copy link
Member

@dhruvmanila dhruvmanila commented Nov 25, 2020

Hey, @cclauss this is creating a new directory in the root.

If he gives an ok then @erdum please add an empty __init__.py file in the directory as well.

@cclauss
Copy link
Member

@cclauss cclauss commented Nov 25, 2020

this is creating a new directory in the root.

Thanks for the heads up on that. I missed that detail and we should seriously consider if making a new root directory is the right thing to do. Would we consider this a conversion -- Not really. What other existing root directories would make sense for this contribution or should we really create a new root dir?

@dhruvmanila
Copy link
Member

@dhruvmanila dhruvmanila commented Nov 25, 2020

If this really needs a new directory then I think it should be physics.

Thinking about it, we can keep the number of directories same by putting the quantum directory in the physics.

@cclauss
Copy link
Member

@cclauss cclauss commented Nov 25, 2020

There are tons of quantum algorithms that have nothing to do with physics.

@cclauss
Copy link
Member

@cclauss cclauss commented Nov 25, 2020

I would be OK with adding electronics (there are tons of possible algorithms in that category) if we could get rid of something else... What about knapsack? That seems to be the weakest topic for a root directory. Where could we stuff the knapsack algorithms to make room for electronics?

https://www.researchgate.net/publication/269211254_The_analysis_and_optimization_algorithms_of_the_electronic_circuits_design

@cclauss cclauss changed the title New algorithm added Ohm's Law algorithm added Nov 25, 2020
Copy link
Member

@cclauss cclauss left a comment

Let's fast-fail.

electronics/ohms_law.py Show resolved Hide resolved
result = {"voltage": float(current * resistance)}
return result
Comment on lines 23 to 24

This comment has been minimized.

@cclauss

cclauss Nov 25, 2020
Member

Simply remove variables that are created on one line and then deleted on the next line.

Suggested change
result = {"voltage": float(current * resistance)}
return result
return {"voltage": float(current * resistance)}

This comment has been minimized.

@cclauss

cclauss Nov 25, 2020
Member

Why do we cast as float() on this branch but not on the other two branches?

This comment has been minimized.

@erdum

erdum Nov 25, 2020
Author Contributor

Because if you divide two values its gives you float but not in case for the multiplication , and remember i give return type float in type hint

result = {"current": voltage / resistance}
return result
Comment on lines 31 to 32

This comment has been minimized.

@cclauss

cclauss Nov 25, 2020
Member

Suggested change
result = {"current": voltage / resistance}
return result
return {"current": voltage / resistance}
result = {"resistance": voltage / current}
return result
Comment on lines 37 to 38

This comment has been minimized.

@cclauss

cclauss Nov 25, 2020
Member

Suggested change
result = {"resistance": voltage / current}
return result
return {"resistance": voltage / current}
@dhruvmanila
Copy link
Member

@dhruvmanila dhruvmanila commented Nov 25, 2020

It seems that the images directory should be removed as we are not using Travis anymore.

@dhruvmanila
Copy link
Member

@dhruvmanila dhruvmanila commented Nov 25, 2020

There's also greedy_knapsack in the greedy_method directory.

This is my proposal:

  • Remove images and greedy_method directory.
  • Move greedy_knapsack to knapsack directory as there are multiple variations for the knapsack problem according to Wikipedia and multiple ways of solving as well.
@erdum
Copy link
Contributor Author

@erdum erdum commented Nov 25, 2020

And now i think you may don't want to add my contribution because i saw other PR just simple and you see it and just merged without any exception but with my case you nearly figured out every small detail, i don't think this is a great place for beginner to start with that's not the way to teach a beginner its just like you are boss and i am your employ, finally i just say what i realize is you have specific problem with me.

@erdum erdum closed this Nov 25, 2020
@erdum
Copy link
Contributor Author

@erdum erdum commented Nov 25, 2020

Very disappointment not expecting from a person who has lot's of experience, i expecting kindness and good teaching attitude but not always people are good

@cclauss cclauss reopened this Nov 25, 2020
@cclauss
Copy link
Member

@cclauss cclauss commented Nov 25, 2020

@erdum I think this PR is extremely close to being landed. I know it is a lot of work but we have a great repo here because we sweat the details. We were seriously overwhelmed by Hacktoberfest so we might have let thru things that were not at the highest level of quality but that is the exception, not the rule.

Thanks massively for your hard work and patience.

@cclauss cclauss merged commit 4191b95 into TheAlgorithms:master Nov 25, 2020
2 checks passed
2 checks passed
build
Details
pre-commit
Details
@erdum
Copy link
Contributor Author

@erdum erdum commented Nov 25, 2020

@cclauss if you all say to me before then i am not going to comment that but sorry if my words hutt you but i jus lost my patience .

@cclauss
Copy link
Member

@cclauss cclauss commented Nov 25, 2020

Patience is a fragile thing. Your algorithm is now part of the repo. Thanks for that!

@erdum
Copy link
Contributor Author

@erdum erdum commented Nov 25, 2020

I am beginner to github its my first time ever contribution i don't even about the CI for testing my code i figured out by my own how to solve issue because my checks are failing but in first time i jus understand that the more your code is clean then more your code is to be good

@cclauss
Copy link
Member

@cclauss cclauss commented Nov 25, 2020

Agreed. Our high standards make this a difficult repo for beginners to contribute to but we are trying to help people to quickly learn things like testing, type hints, good variable names, f-strings because they can make code more readable and more reliable. Other repos can be easier to contribute to but this repo tries to teach best practices.

@erdum
Copy link
Contributor Author

@erdum erdum commented Nov 25, 2020

yes I learned a lot from you and your repo thanks

@cclauss
Copy link
Member

@cclauss cclauss commented Nov 25, 2020

The thing that I love is that this is not my repo... It is our repo. Now that your algorithm is in the repo, you are part of the club. The electronics directory currently only has one algorithm so please consider what else could be added.

@erdum
Copy link
Contributor Author

@erdum erdum commented Nov 25, 2020

Yes my next goal will be on kcl law, it is complicated so i will try to run it locally then i will make PR so you can test or debug my code

@erdum
Copy link
Contributor Author

@erdum erdum commented Nov 25, 2020

Now i realize you are a very nice person actually, so sorry for the previous comments

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
4 participants
You can’t perform that action at this time.