-
Notifications
You must be signed in to change notification settings - Fork 105
Feature/yo filtered double #214
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
base: develop
Are you sure you want to change the base?
Conversation
|
@SylvainBertrand let me know if there are any ways I could make this more useful or accessible. Ideally, this work can generalize to any type of causal filter, and perhaps it would be more useful if I made some example classes where people wouldn't need to know or setup the transfer function beforehand. |
427c547 to
c6fab68
Compare
|
@rjgriffin42 original request was nearly a year ago now lol |
|
@rjgriffin42 1.5 years ago, but I could really use this now |
| this.name = name; | ||
| this.k = k; | ||
| this.numerator = numerator; | ||
| this.denominator = denominator; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer these to be copies, so that this doesn't keep track of the pointer.
| this.name = name; | ||
|
|
||
| int numOfTransferFunctionsToCascade = severalContinuousTransferFunctions.length; | ||
| ContinuousTransferFunction tf_total; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
resultingTransferFunction
| // Set private variables to total transfer function variables; | ||
| this.k = tf_total.getGain(); | ||
| this.numerator = tf_total.getNumerator(); | ||
| this.denominator = tf_total.getDenominator(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should also be a copy, not a return
| * @param H2 - Transfer Function 2 | ||
| * @return H3 - Combined Transfer Function of 1 and 2 (H3(s) = H1(s)*H2(s)) | ||
| */ | ||
| private ContinuousTransferFunction CascadeTwoTransferFunctions(ContinuousTransferFunction H1, ContinuousTransferFunction H2) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why cascade? why not multiply? At the very least, this should be camelCase, not PascalCase.
I would also say that you should make a static method as:
private static void multiply(ContinuousTransferFunction resultToPack, ContinuousTransferFunction H1, ContinuousTransferFunction H2)
{
resultToPack.setGain(H1.getGain() * H2.getGain());
multiplyPolynomial(resultToPack.getNumeratorUnsafe(), H1.getNumerator(), H2.getNumerator());
multiplyPolynomial(resultToPack.getDenominatorUnsafe(), H1.getDenominator(), H2.getDenominator());
}
Then change the body of this method to:
ContinuousTransferFunction result = new ContinuousTransferFunction();
multiply(result, H1, H2);
return result;
You will also need to then rename the methods getNumerator() and getDenominator() to getNumeratorUnsafe() and getDenominatorUnsafe() to recognize that they're returning a pointer to the object contained in this class.
You also need to add a warning in this javadoc that it is not garbage free.
|
|
||
| public double[] getDenominator() | ||
| { | ||
| return denominator; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rename these methods to **Unsafe() and then add these as
public void getNumerator(double[] numeratorToPack)
{
copy
}
| * @param poly2 | ||
| * @return poly3 = poly1*poly2 | ||
| */ | ||
| private static double[] cascadePolynomials(double[] poly1, double[] poly2) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should be changed to
private static double[] multiplyPolynomials(double[] poly1, double[] poly2)
{
double[] result = new double[poly1.length + poly2.length - 1];
multiply(result, poly1, poly2);
return result;
}
private static void multiplyPolynomials(double[] resultToPack, double[] poly1, double poly2)
{
// Cycle through poly1 and poly2 to polynomials.
for (int i = 0; i < poly1.length; i++)
{
for (int j = 0; j < poly2.length; j++)
{
result[i + j] += poly1[i] * poly2[j];
}
}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and for the first one, you need to add a warning that it allocates and is not garbage free.
|
|
||
| public void setDenominator(double[] denominator) | ||
| { | ||
| this.denominator = denominator; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would make these copy value, rather than store the pointer, so that it doesn't get modified unintentionally.
| * | ||
| * @author Connor Herron | ||
| */ | ||
| public class ContinuousTransferFunction |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add some static methods that create transfer functions for generic types. Example,
createLowPassFilterTransferFunction(double breakFrequency)createHighPassFilterTransferFunction(double breakFrequency)createNotchFilterTransferFunction(double notchFrequency, double notchWidth)
| private String name; | ||
| private double k; | ||
| private double[] numerator; | ||
| private double[] denominator; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can these be final?
| this.name = name; | ||
|
|
||
| m = tf.getNumerator().length; | ||
| n = tf.getDenominator().length; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these may need to change, or be direct methods (getNumberOfPoles(), getNumberOfZeroes())
|
|
||
| this.k = tf.getGain(); | ||
| this.fs = sampleFrequency; | ||
| this.p = 1 / (2 * this.fs); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this.p = 1.0 / (2.0 * sampleFrequency);
|
|
||
| private void buildFilter() | ||
| { | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove line
|
|
||
| private double calc; | ||
|
|
||
| private DMatrixRMaj solveFx_to_Fz(DMatrixRMaj Fx) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is full of garbage creation. Please fix it. I also have no idea what this method does.
| denominator = solveFx_to_Fz(denominator); | ||
|
|
||
| // Solve for outputCoefficients. | ||
| inputCoefficients = numerator.copy(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
garbage
| CommonOps_DDRM.scale(1.0 / denominator.get(0), inputCoefficients); | ||
|
|
||
| // Solve for inputCoefficients. | ||
| double[] d = new double[n - 1]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
garbage
| // Solve for inputCoefficients. | ||
| double[] d = new double[n - 1]; | ||
| System.arraycopy(denominator.data, 1, d, 0, denominator.numCols - 1); | ||
| outputCoefficients = new DMatrixRMaj(1, d.length, false, d); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
garbage
| { | ||
| outputVec.set(i, inputVec.get(numCols - 1 - i)); | ||
| } | ||
| return outputVec; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you not do a swap operation on the entries of the backing array, instead?
double[] data = inputVector.data;
int lastIndex = inputVector.data.length - 1;
for (int i = 0; i < numCols / 2; i++)
{
double newStart = inputVector[lastIndex - i];
inputVector[lastIndex - i] = inputVector[i];
inputVector[i] = newStart;
}
| public DMatrixRMaj getOutputCoefficients() | ||
| { | ||
| return outputCoefficients.copy(); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should do a pack operation here instead.
| private DMatrixRMaj syntheticDivision(DMatrixRMaj inputVec, double sign) | ||
| { | ||
| numCols = inputVec.numCols; | ||
| tempVec = inputVec; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is going on here? This doesn't copy, so tempVec is always equal to inputVec, so I'm sure this is doing what you think.
| calc *= p; | ||
| Fx.times(i, calc); | ||
| } | ||
| Fx = reverseVector(Fx); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be done in place, so it doesn't generate garbage.
| Fx = reverseVector(Fx); | ||
|
|
||
| // 4) Convert F(1/x + 1) -> F(2/x + 1). | ||
| calc = 1.0 / 2.0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why would you write it this way, as opposed to 0.5? This does an actual operation, while writing 0.5 doesn't.
Original PR: #197
I had to cherry-pick the actual commits out of the branch.