Issue 10 partial fraction class#14
Open
Jared-Gibson wants to merge 13 commits into
Open
Conversation
-added methods that I believe will be useful in creating a Partial Fraction Decomposition class
-Added the work-flow for PartialFracDecomp -Added implementation for splitEquationString
-renamed factorDenom to factorExpr -changed return value of factorExpr from a string to use a more useful representation of an equation
-added code for finding LHS using Algebra class (waiting on pull requests) -added code for finding RHS using Algebra class (waiting on pull requests)
-LHS calculation was incorrect; numerator just needs to have all its factors multiplied -fixed error in matrix resizing; the height of the matrix is determined by the largest power of the denominator not the numerator
-iterates through RHSexpressions -for each string in RHSexpressions, we iterate backwards since the constant will always be on the right hand side of the expression; convert the characters into an int to store into RHScoeff
-uses similar logic to RHS, just without the outer for-loop -sketching out the matrix format
-added for-loops that add expression coefficients to a 2x2 integer matrix
…the unit testing suite
-This method does not fully work as it requires the Algebra class which is not implemented at this time; however, I wrote a test case for it anyway
theEhlien
reviewed
May 4, 2020
Member
theEhlien
left a comment
theEhlien
left a comment
There was a problem hiding this comment.
Looks good! The tests all ran for me as well and the code that you prepared for when the Algebra class is available also looks good. I didn't see any obvious bugs and your commenting looks consistent and clean.
|
Tests all worked fine, good documentation, class functions look great. This looks ready to add on |
|
Just finished running the code, and it ran smoothly for me as well! I appreciated how easy the comments were to follow, as well as the depiction of the matrix represented as comments, nicely done! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description of the changes:
-Added framework for a partial fraction decomposition class
-Two methods have been implemented: splitEquationString and createCoeffMatrix
-splitEquationString is working as intended & is unit tested
-createCoeffMatrix cannot be tested as it relies on the Algebra class (which hasn't been merged into master yet); I did add unit testing for this method when Algebra does get merged
-All other methods that have not been implemented throw a "Not implemented" exception