Skip to content

Enhanced the code #5330

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

Closed
wants to merge 6 commits into from
Closed

Enhanced the code #5330

wants to merge 6 commits into from

Conversation

priyaa-rawat
Copy link

Describe your change:

Added an exception and a new method.

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

Checklist:

  • [x ] I have read CONTRIBUTING.md.
  • This pull request is all my own work -- I have not plagiarized.
  • [ x] 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.
  • [x ] All filenames are in all lowercase characters with no spaces or dashes.
  • [ x] 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}.
    Hacktoberfest 2021 - Implement algorithms or data structures, fix existing ones and more! #4996 Update: graph_matrix #5318 @cclauss

@ghost ghost added awaiting reviews This PR is ready to be reviewed enhancement This PR modified some existing files tests are failing Do not merge until tests pass labels Oct 15, 2021
@cclauss
Copy link
Member

cclauss commented Oct 16, 2021

Under the Details next to the ❌ below, psf/black say:

-        if u>self.vertex or v>self.vertex or u<1 or v<1:
+        if u > self.vertex or v > self.vertex or u < 1 or v < 1:

so please add spaces to let your code breathe. As discussed in CONTRIBUTING.md, code that is not black formatted will not be accepted.

@priyaa-rawat
Copy link
Author

@cclauss I did the changes and formatted the code with black too. I hope there won't be any other errors. Since Iam a beginner Iam not able to get them. Thanku for the guidance .

@ghost ghost removed the tests are failing Do not merge until tests pass label Oct 16, 2021
@priyaa-rawat
Copy link
Author

@poyea @cclaus please review my PR

@cclauss
Copy link
Member

cclauss commented Oct 17, 2021

So… There are no doctests or type hints as discussed in CONTRIBUTING.md. This code confuses two kinds of graphs.

Vertex and show() refer to an x, y chart that is plotted. In this sense, vertex is misnamed and should be named size or dimension as it represents the maximum value for x and/or y.

The rest of the code is focused on a graph that is a set of nodes connected by edges. This can be proven by looking at the output of .show().

Which of these two graphs should we focus on?!?

@priyaa-rawat
Copy link
Author

@cclaus I went through all the algorithms and files and this one code had no doctests or type hints originally like most of the files in the folder so I didnt to avoid any errors.
-As for the variable vertex and .show it was how the code was from the beginning so I didn't alter it though it was confusing to me too.

I just added a function remove edge and an if condition to raise an error if the index goes out of range.

-This code is supposed to focus on nodes and edges according to what I understood.

@cclauss
Copy link
Member

cclauss commented Oct 17, 2021

All files that start with b (i.e. graphs/b*.py) have doctests.

@cclauss cclauss closed this Oct 17, 2021
@priyaa-rawat priyaa-rawat deleted the priyaa-rawat-patch-1 branch October 17, 2021 15:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting reviews This PR is ready to be reviewed enhancement This PR modified some existing files
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants