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

Update evaluate_postfix_notations.py #8758

Closed
wants to merge 16 commits into from

Conversation

rohan472000
Copy link
Contributor

@rohan472000 rohan472000 commented May 22, 2023

Describe your change:

Fixes #8754

  • 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 include at least one URL that points to Wikipedia or another similar explanation.
  • If this pull request resolves one or more open issues then the commit message contains Fixes: {#8754}.

@algorithms-keeper algorithms-keeper bot added the tests are failing Do not merge until tests pass label May 22, 2023
@algorithms-keeper algorithms-keeper bot added the awaiting reviews This PR is ready to be reviewed label May 22, 2023
@rohan472000 rohan472000 changed the title Update evaluate_postfix_notations.py Update evaluate_postfix_notations.py mentioned in #8754 issue May 22, 2023
@amirsoroush
Copy link
Contributor

Hi, Please add Fixes #8754 to the PR's description so that GitHub can attach this PR to the issue.

Copy link
Contributor

@amirsoroush amirsoroush left a comment

Choose a reason for hiding this comment

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

I couldn't pass the doctests. Here are two failed test cases:

**********************************************************************
File ".../evaluate_postfix_notations.py", line 18, in __main__.evaluate_postfix
Failed example:
    evaluate_postfix(["4", "13", "5", "/", "+"])
Expected:
    6
Got:
    6.6
**********************************************************************
File ".../evaluate_postfix_notations.py", line 20, in __main__.evaluate_postfix
Failed example:
    evaluate_postfix(["2", "+"])
Exception raised:
    Traceback (most recent call last):
      File "/usr/lib/python3.10/doctest.py", line 1350, in __run
        exec(compile(example.source, filename, "single",
      File "<doctest __main__.evaluate_postfix[2]>", line 1, in <module>
        evaluate_postfix(["2", "+"])
      File ".../evaluate_postfix_notations.py", line 46, in evaluate_postfix
        raise ValueError(
    ValueError: Invalid expression: insufficient operands for binary operator
**********************************************************************
1 items had failures:
   2 of   5 in __main__.evaluate_postfix
***Test Failed*** 2 failures.

>>> evaluate_postfix([])
0
"""
if not postfix_notation:
return 0

operations = {"+", "-", "*", "/"}
unary_operations = {"+"} # Unary operator(s)
Copy link
Contributor

Choose a reason for hiding this comment

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

unary_operations is not currently being used.

Copy link
Contributor Author

@rohan472000 rohan472000 Jun 11, 2023

Choose a reason for hiding this comment

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

now I have made several changes, kindly look into it.

Comment on lines 38 to 41
if len(stack) < 1:
raise ValueError(
"Invalid expression: insufficient operands for unary operator"
)
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't fix the problem with unary negation operators. We want to support negation, not raise an error if it's present.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

made several changes, look into that.

@rohan472000 rohan472000 changed the title Update evaluate_postfix_notations.py mentioned in #8754 issue Update evaluate_postfix_notations.py Jun 11, 2023
@algorithms-keeper algorithms-keeper bot removed the tests are failing Do not merge until tests pass label Jun 11, 2023
Comment on lines 38 to 40
elif (True) and len(stack) < 2:
operand = stack.pop()
stack.append(operand)
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the purpose of this elif block?

Copy link
Contributor Author

@rohan472000 rohan472000 Jun 11, 2023

Choose a reason for hiding this comment

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

This block will handle +,*,/ operators as above block(if block) is handling "-" operator.

@rohan472000 rohan472000 requested a review from amirsoroush June 11, 2023 17:45
Copy link
Contributor

@amirsoroush amirsoroush left a comment

Choose a reason for hiding this comment

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

I guess that way is more readable.

if token == "-" and len(stack) < 2:
operand = stack.pop()
stack.append(-operand)
elif (True) and len(stack) < 2:
Copy link
Contributor

Choose a reason for hiding this comment

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

elif (True) and len(stack) < 2 can be simplified to just len(stack) < 2.

Co-authored-by: AmirSoroush <114881632+amirsoroush@users.noreply.github.com>
@rohan472000
Copy link
Contributor Author

rohan472000 commented Jun 12, 2023

@cclauss, look into it again in order to merge.

Copy link
Member

@cclauss cclauss left a comment

Choose a reason for hiding this comment

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

Doing this work without using https://docs.python.org/3/library/operator.html seems suboptimal.

We should be able to deal with both floats and ints.

Please add tests for negative numbers, zeros, floats and

  • evaluate_postfix(["5"])
  • evaluate_postfix(["-"])
  • evaluate_postfix(["5", "A"])
  • evaluate_postfix(["5", "4", "A"])
  • evaluate_postfix(["A"])

rohan472000 and others added 2 commits June 12, 2023 17:04
Co-authored-by: Christian Clauss <cclauss@me.com>
Co-authored-by: Christian Clauss <cclauss@me.com>
@algorithms-keeper algorithms-keeper bot added the tests are failing Do not merge until tests pass label Jun 12, 2023
Co-authored-by: Christian Clauss <cclauss@me.com>
@rohan472000
Copy link
Contributor Author

for >>> evaluate_postfix(["-"]) what output should I give, 0 or -1 or anything else??

@tianyizheng02
Copy link
Contributor

for >>> evaluate_postfix(["-"]) what output should I give, 0 or -1 or anything else??

@rohan472000 See my comment above. It should raise an error because the negation operator has nothing to negate.

@algorithms-keeper algorithms-keeper bot removed the tests are failing Do not merge until tests pass label Jun 12, 2023
Comment on lines 35 to 41
if len(stack) < 2:
operand = stack.pop()
if token == "-":
operand = -operand
else:
raise ValueError(f"Unrecognized {token = }")
stack.append(operand)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if len(stack) < 2:
operand = stack.pop()
if token == "-":
operand = -operand
else:
raise ValueError(f"Unrecognized {token = }")
stack.append(operand)
if token == "-":
if len(stack) == 1:
operand = stack.pop()
stack.append(-operand)
elif len(stack) == 0:
raise ValueError(f"Negation operator with no operand")

Currently, if there's only 1 operand on the stack but the token is not -, then it just appends the operand back onto the stack. This effectively leaves the stack exactly as it already was, so you can skip that altogether.

Furthermore, checking len(stack) < 2 includes the case in which the stack is empty. This should raise an error since there's no operand to negate.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not able to commit above change requested...!!

rohan472000 and others added 2 commits June 13, 2023 00:58
Co-authored-by: Tianyi Zheng <tianyizheng02@gmail.com>
Co-authored-by: Tianyi Zheng <tianyizheng02@gmail.com>
@algorithms-keeper algorithms-keeper bot added tests are failing Do not merge until tests pass and removed tests are failing Do not merge until tests pass labels Jun 12, 2023
@rohan472000
Copy link
Contributor Author

rohan472000 commented Jun 13, 2023

@tianyizheng02 ,as your suggestion is making the build fail(doctest not passing), so I have again pushed my previous code.

@tianyizheng02
Copy link
Contributor

The build was failing because this test was failing:

>>> evaluate_postfix(["2", "+"])
UNEXPECTED EXCEPTION: IndexError('pop from empty list')
Traceback (most recent call last):
  File "/opt/hostedtoolcache/Python/3.11.3/x64/lib/python3.11/doctest.py", line 1351, in __run
    exec(compile(example.source, filename, "single",
  File "<doctest data_structures.stacks.evaluate_postfix_notations.evaluate_postfix[2]>", line 1, in <module>
  File "/home/runner/work/Python/Python/data_structures/stacks/evaluate_postfix_notations.py", line 55, in evaluate_postfix
    b, a = stack.pop(), stack.pop()
                        ^^^^^^^^^^^
IndexError: pop from empty list
/home/runner/work/Python/Python/data_structures/stacks/evaluate_postfix_notations.py:20: UnexpectedException

I believe my suggestion was still correct, but you need to add an if-statement to handle this edge case as well. This test should raise an exception because + is a binary operator and there is only 1 operand.

@rohan472000
Copy link
Contributor Author

rohan472000 commented Jun 13, 2023

ok...but main concern is for >>> evaluate_postfix(["-"]) case....which is still not resolved..

@tianyizheng02
Copy link
Contributor

tianyizheng02 commented Jun 13, 2023

ok...but main concern is for >>> evaluate_postfix(["-"]) case....which is still not resolved..

I believe that test should've already been passing. Again, my suggestion (specifically the elif len(stack) == 0: line) should've handled it.

@rohan472000 rohan472000 reopened this Jun 20, 2023
@rohan472000 rohan472000 requested a review from cclauss June 20, 2023 06:21
@tianyizheng02
Copy link
Contributor

Issue has already been fixed by another PR

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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Need to support for unary operator in postfix evaluation.
4 participants