Skip to content

Conversation

ArCh12312
Copy link

#66 Fixed the issue.
Changed assert_almost_equals to assert_allclose and a custom function(assert_decimal_close).
Had to create assert_decimal_close, which finds the absolute difference in values and compares it to a tolerance level.
Cannot use the assert_allclose because it uses infinite under the covers which does not support the Decimal type.
See issue: numpy/numpy#9954
In addition, ran ruff format test_financial.py

@ArCh12312
Copy link
Author

Hey @Kai-Striega!
Sorry, I had to create a new pull request. This was because I thought my branch had some merge issues but turns out it was because I had not seen the new test case.
Please review it.

@Kai-Striega
Copy link
Member

Hello @ArCh12312 firstly my apologies for taking so long to reply, I was on holiday over Christmas/New Year's and didn't check my GitHub. I'm back and should be quicker to respond from now on.

I'll review this over the weekend. Thanks for being patient.

@ArCh12312
Copy link
Author

Hey, @Kai-Striega! Wish you a very happy new year! Thank you for the note.

@Kai-Striega Kai-Striega added this to the 2.0 milestone Jan 6, 2024
Copy link
Member

@Kai-Striega Kai-Striega left a comment

Choose a reason for hiding this comment

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

LGTM.

Just as an aside, could you refrain from running ruff format over the entire file? It makes it harder for me to review and makes git blame messier.

Comment on lines +20 to +22
if hasattr(actual, "__iter__") and hasattr(expected, "__iter__"):
for a, e in zip(actual, expected):
assert abs(a - e) <= tol
Copy link
Member

Choose a reason for hiding this comment

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

This is an elegant solution!

@ArCh12312
Copy link
Author

Thank you for the approval! I will make sure to use Ruff only the necessary bits from next time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants