[feat] Implement PolynomialTraits for Polynomial

[shri-acha]

Currently the implementation of PolynomialTraits for Polynomial is missing.
This PR intends to solve the issue: #65
By implementing the PolynomialTraits for Polynomial

[dawnandrew100]

When PolynomialTraits is eventually implemented for the Polynomial struct, I can see that this evaluation function has almost the same signature as the eval_multivariate function, so it can be a drop in function (similar to that of the IntermediatePolynomial struct; func and impl), but for the eval_univariate function, a similar work around will probably need to be added where the struct itself holds all of the unique variables and if the length of the unique variables is greater than 1, then an error is returned.

And for the evaluation function itself, an error should be returned if a variable in the struct is not defined in the variables map parameter of the function.

So to kind of clarify the intent of this issue/PR, the logic used to get the end result can and should be unique to the struct at hand, but the result returned should be the same as PolynomialTraits so that the function can be used in the implementation.

I know that working on parsing, evaluation, derivation, and integration at once can be challenging, so initially, you can do this

impl Polynomial{
    fn parse(input: &str) -> Result<Self, PolynomialError> { }
    fn eval_univariate<F>(&self, point: F) -> Result<f64, PolynomialError> { }
    fn eval_multivariate<V, S, F>(&self, vars: &V) -> Result<f64, PolynomialError> { }
}

as you're working through the individual parts

But, in the end, in the structs/advanced.rs file, there should be this

impl PolynomialTraits for Polynomial{
    fn parse(input: &str) -> Result<Self, PolynomialError> { }
    fn eval_univariate<F>(&self, point: F) -> Result<f64, PolynomialError> { }
    fn eval_multivariate<V, S, F>(&self, vars: &V) -> Result<f64, PolynomialError> { }
    fn derivate_univariate(&self) -> Result<Self, PolynomialError> { }
    fn derivate_multivariate<S>(&self, var: S) -> Self { }
    fn indefinite_integral_univariate(&self) -> Result<Self, PolynomialError> { }
    fn indefinite_integral_multivariate<S>(&self, var: S) -> Self { }
}

That's so that this struct is able to be used in the math portion of this crate without needing to write special work arounds for each of the three available structs.

[dawnandrew100]

The eval function should output a value, not a struct

fn eval_univariate<F>(&self, point: F) -> Result<f64, PolynomialError> {}
fn eval_multivariate<V, S, F>(&self, vars: &V) -> Result<f64, PolynomialError> {}

[shri-acha]

I do understand what you mean with the variable field in the Polynomial, But I think there is also another way of implementing this, which is by partial application of the variables, and this would be able to trace lack of sufficient variables in case of multivariate or univariate expressions.
i.e.
for: x^2 + 2xy
if x = 5, 5^2+2*5*y
and there would be a pass that would check if there's any remaining variables, if so, then
if eval_multivariate is called, then an error is thrown
if eval_univariate is called, then an error is thrown
Similarly,
for: x^2 + 2x
if (x,y) = (5,7), 5^2+2*5
if eval_multivariate is called, it doesn't fail.
if eval_univariate is called, it doesn't fail.
But this would mean that we'd essentially be allowing cases of extra variables in the varmap.

Each pass iterates through each variable mapping and gives us an expression with substituted value and a final pass checks for above mentioned conditions and gives us a result.

Let me know what you think of this idea @dawnandrew100.

[dawnandrew100]

@shri-acha It sounds like you want to iterate over the entire parsed expression for each variable to swap it with the relevant value. So, for example, with your second example, you'd iterate over x^2 + 2x to see if there are any x's that can be swapped with 5 and then you'd iterate over 5^2 + 2*5 to see if there are any y's that can be swapped with 7.

Instead, would it be possible to go through the parsed AST directly and if there's a variable encountered, you can .get() the associated value from the varmap and if the .get() fails then it returns an error. So, instead of iterating through the entire expression for each variable in the varmap (which can get complicated with more complex ASTs) and again to make sure that there aren't any leftover variables, you just iterate through the AST once.

With eval_univariate specifically, there's no parameter to specify which variable is being evaluated, which is why there needs to be some mechanism to make sure that the actual polynomial expression only has one unique variable before being evaluated. In this case, one pass through the expression to make sure that there's only one unique variable (or return an error if there's more than one) and then another pass to substitute the single variable with the value would make sense. So, for this function, instead of saying that x=5, the parameter would just be 5 and it would be assigned to whatever variable is present in the polynomial (providing there is only one variable).

But, in the eval_multivariate, I'm not sure that there should be a penalty if there are too many variables in the varmap. Because, in wolfram alpha, for example, if you were to type Power[x,3]+5 where x=5, y=10, it just substitutes x with 5 and evaluates to130 and completely ignores the y=10 assignment.

[shri-acha]

@dawnandrew100, I'm not sure what base for the Log function is here, I've assumed for it to be 10 and I've gone ahead and changed the current implementation currently as you requested it to be and will be adding the implementation for multi and univariate cases.

[dawnandrew100]

@shri-acha
For right now we can assume the base for the log function is 10. In the future, we'll look into specifying different bases of logs, but for now the base for the Log function can just be 10.

Also, I noticed that statrs was added as a dependency. Is there a way that we can implement whatever functionality was needed from that crate? I'm trying to minimise the number of dependencies this crate relies on.

[shri-acha]

yeah, i forgot to get rid of the dependency, I just implemented it by myself.

[dawnandrew100]

Everything's looking good so far! In addition to the two minor changes, tests including evaluation of functions and constants should be added to make sure that those work as expected as well.

[dawnandrew100]

@shri-acha
When it comes to implementing the rules for differentiation, I know that the multiplication by a constant, power rule, sum rule, and difference rule will be relatively straight forward to implement. Do you think there's going to be any trouble implementing the more complicated rules like the product rule, quotient rule, or chain rule?

And with the rules for integration, how do you best think we should represent the + C?

Requested changes added to PR

[shri-acha]

When it comes to implementing the rules for differentiation, I know that the multiplication by a constant, power rule, sum rule, and difference rule will be relatively straight forward to implement. Do you think there's going to be any trouble implementing the more complicated rules like the product rule, quotient rule, or chain rule?

I mean, with our current architecture, I believe implementation of product or quotient rule should be straight forward. We can just walk through the AST and replace or just build a new equation with traversal. I'll try to look if I can minimize the creations with just mutating the existing equation. i'm not totally sure about it though.

And with the rules for integration, how do you best think we should represent the + C?

I think we could just add a constant, called maybe 'C' with an arbitrary value.

[dawnandrew100]

@shri-acha Sounds good!
I'm trying to think of the best way to integrate +C into the project without eliminating the ability to use c as a variable. I think just adding a token called IntegrationConstant or PlusC or something of that nature would be good

#[derive(Debug, PartialEq, Clone)]
pub enum Token {
    Number(f64),
    Variable(String),
    Operator(Operators),
    Function(Functions),
    Constant(Constants),
    LParen,
    RParen,
    IntegrationConstant, <-
}

Then, when the expression is being evaluated, there can be an optional way to define this constant such that it can be included in the calculation OR if it's not defined, it can just be ignored (ie C = 0).
There should also be some way to keep track of multiple integration constants, because if an expression that has already been integrated is integrated again, a new independent integration constant is added. So, if $ax + C$ is integrated, instead of having $\frac{1}{2}ax^2 + Cx + C$, we need to have something like $\frac{1}{2}ax^2 + C_1 x + C_2$.

[shri-acha]

You're right, I'll try looking into something that would work for this scenario, maybe we could add a integration pool while integration and have a state track the integration constant taken out. Which would simplify the process of state tracking for the integration constant.

[dawnandrew100]

@shri-acha in an effort to merge this PR without this single PR having to deal with all the nuances of all the functions in Polynomial traits, I subdivided issue #65 into 4 issues: #90, #91, #92, and #93.
This PR addresses the parsing and evaluation functions of #90 and #91.

You're welcome to also tackle the other issues as well, but I'll merge this PR in the context of it addressing two of those four parts of issue #65.

[dawnandrew100]

Instead of cloning the entire expression tree, is there another way to traverse the tree without needing to clone (like using the visitor pattern for example)

[dawnandrew100]

Is there a way to remove this clone?

[dawnandrew100]

We should also probably take a look at the scoping of these various functions. Like this function might be good as pub(crate) instead of just pub, but this can be discussed whether that's a good idea or not

[shri-acha]

yeah, I mean, are we expecting on introducing the api seperately? If so, then this would be fine, else I'll have to scope it down for pub (crate).

[dawnandrew100]

What specifically do you mean by introducing the API separately??

[dawnandrew100]

We might be able to replace this clone too if we implement the visitor pattern mentioned in one of my other comments

[dawnandrew100]

Another clone that might be removed

[shri-acha]

i'm sorry i have been a little too busy currently because of my academics, i'll try to get back to it when I can. As for the reviews for the current PR, I'll try to work on it soon.

[dawnandrew100]

@shri-acha no worries! I've also been busy over the past month so haven't had a ton of time to really do any coding but I'm trying to get back into the swing of things

I'll also start working on solving some of the comments so that we can get this PR merged hopefully within the next few days