Skip to content

Implement Sub trait for LinearMonomial, QuadraticMonomial, and MonomialDyn #621

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.

Sign up for GitHub

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

Open
termoshtt wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Implement Sub trait for LinearMonomial, QuadraticMonomial, and MonomialDyn #621

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
93bb271
Implement Sub trait for LinearMonomial, QuadraticMonomial, and Monomi…
termoshtt Aug 26, 2025
805d953
Refactor monomial operation macros to reduce code duplication
termoshtt Aug 27, 2025
684d128
Address PR feedback: clean up macro parameters and comments
termoshtt Aug 27, 2025
408bf11
Merge branch 'main' into monomial-sub
termoshtt Aug 27, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
150 changes: 107 additions & 43 deletions rust/ommx/src/polynomial_base/add.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,99 +96,163 @@ impl<M: Monomial> Add<&M> for &PolynomialBase<M> {
}
}

// Add support for Monomial + Monomial operations for specific monomial types
macro_rules! impl_monomial_add {
($monomial:ty) => {
impl Add for $monomial {
// Add missing Sub<Coefficient> operations
Copy link

Copilot AI Aug 27, 2025

Choose a reason for hiding this comment

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

[nitpick] The comment suggests these operations were 'missing', but this appears to be new functionality being added. Consider updating the comment to 'Add Sub operations' for clarity.

Suggested change
// Add missing Sub<Coefficient> operations
// Add Sub<Coefficient> operations

Copilot uses AI. Check for mistakes.
impl<M: Monomial> Sub<Coefficient> for PolynomialBase<M> {
type Output = Self;
fn sub(mut self, rhs: Coefficient) -> Self::Output {
self -= rhs;
self
}
}

impl<M: Monomial> Sub<PolynomialBase<M>> for Coefficient {
type Output = PolynomialBase<M>;
fn sub(self, mut rhs: PolynomialBase<M>) -> Self::Output {
rhs = -rhs;
rhs += self;
rhs
}
}

impl<M: Monomial> Sub<Coefficient> for &PolynomialBase<M> {
type Output = PolynomialBase<M>;
fn sub(self, rhs: Coefficient) -> Self::Output {
self.clone() - rhs
}
}

impl<M: Monomial> Sub<&PolynomialBase<M>> for Coefficient {
type Output = PolynomialBase<M>;
fn sub(self, rhs: &PolynomialBase<M>) -> Self::Output {
self - rhs.clone()
}
}

// Generic macro for implementing binary operations for monomial types
macro_rules! impl_monomial_op {
($op_trait:ident, $op_method:ident, $monomial:ty) => {
impl $op_trait for $monomial {
type Output = PolynomialBase<$monomial>;
fn add(self, rhs: Self) -> Self::Output {
PolynomialBase::from(self) + PolynomialBase::from(rhs)
fn $op_method(self, rhs: Self) -> Self::Output {
PolynomialBase::from(self).$op_method(PolynomialBase::from(rhs))
}
}

impl Add<&$monomial> for $monomial {
impl $op_trait<&$monomial> for $monomial {
type Output = PolynomialBase<$monomial>;
fn add(self, rhs: &Self) -> Self::Output {
PolynomialBase::from(self) + PolynomialBase::from(rhs.clone())
fn $op_method(self, rhs: &Self) -> Self::Output {
PolynomialBase::from(self).$op_method(PolynomialBase::from(rhs.clone()))
}
}

impl Add<$monomial> for &$monomial {
impl $op_trait<$monomial> for &$monomial {
type Output = PolynomialBase<$monomial>;
fn add(self, rhs: $monomial) -> Self::Output {
PolynomialBase::from(self.clone()) + PolynomialBase::from(rhs)
fn $op_method(self, rhs: $monomial) -> Self::Output {
PolynomialBase::from(self.clone()).$op_method(PolynomialBase::from(rhs))
}
}

impl Add for &$monomial {
impl $op_trait for &$monomial {
type Output = PolynomialBase<$monomial>;
fn add(self, rhs: Self) -> Self::Output {
PolynomialBase::from(self.clone()) + PolynomialBase::from(rhs.clone())
fn $op_method(self, rhs: Self) -> Self::Output {
PolynomialBase::from(self.clone()).$op_method(PolynomialBase::from(rhs.clone()))
}
}

// Add support for Monomial + Coefficient operations
impl Add<Coefficient> for $monomial {
// Operations with Coefficient
impl $op_trait<Coefficient> for $monomial {
type Output = PolynomialBase<$monomial>;
fn add(self, rhs: Coefficient) -> Self::Output {
PolynomialBase::from(self) + rhs
fn $op_method(self, rhs: Coefficient) -> Self::Output {
PolynomialBase::from(self).$op_method(rhs)
}
}

impl Add<$monomial> for Coefficient {
impl $op_trait<$monomial> for Coefficient {
type Output = PolynomialBase<$monomial>;
fn add(self, rhs: $monomial) -> Self::Output {
self + PolynomialBase::from(rhs)
fn $op_method(self, rhs: $monomial) -> Self::Output {
self.$op_method(PolynomialBase::from(rhs))
}
}

impl Add<Coefficient> for &$monomial {
impl $op_trait<Coefficient> for &$monomial {
type Output = PolynomialBase<$monomial>;
fn add(self, rhs: Coefficient) -> Self::Output {
PolynomialBase::from(self.clone()) + rhs
fn $op_method(self, rhs: Coefficient) -> Self::Output {
PolynomialBase::from(self.clone()).$op_method(rhs)
}
}

impl Add<&$monomial> for Coefficient {
impl $op_trait<&$monomial> for Coefficient {
type Output = PolynomialBase<$monomial>;
fn add(self, rhs: &$monomial) -> Self::Output {
self + PolynomialBase::from(rhs.clone())
fn $op_method(self, rhs: &$monomial) -> Self::Output {
self.$op_method(PolynomialBase::from(rhs.clone()))
}
}

// Add support for Monomial + PolynomialBase operations
impl Add<PolynomialBase<$monomial>> for $monomial {
// Operations with PolynomialBase
impl $op_trait<PolynomialBase<$monomial>> for $monomial {
type Output = PolynomialBase<$monomial>;
fn add(self, mut rhs: PolynomialBase<$monomial>) -> Self::Output {
rhs.add_term(self, coeff!(1.0));
rhs
fn $op_method(self, rhs: PolynomialBase<$monomial>) -> Self::Output {
// Special handling for different operation types
impl_monomial_op!(@internal $op_trait, $op_method, self, rhs)
}
}

impl Add<&PolynomialBase<$monomial>> for $monomial {
impl $op_trait<&PolynomialBase<$monomial>> for $monomial {
type Output = PolynomialBase<$monomial>;
fn add(self, rhs: &PolynomialBase<$monomial>) -> Self::Output {
self + rhs.clone()
fn $op_method(self, rhs: &PolynomialBase<$monomial>) -> Self::Output {
self.$op_method(rhs.clone())
}
}

impl Add<PolynomialBase<$monomial>> for &$monomial {
impl $op_trait<PolynomialBase<$monomial>> for &$monomial {
type Output = PolynomialBase<$monomial>;
fn add(self, mut rhs: PolynomialBase<$monomial>) -> Self::Output {
rhs.add_term(self.clone(), coeff!(1.0));
rhs
fn $op_method(self, rhs: PolynomialBase<$monomial>) -> Self::Output {
// Special handling for different operation types
impl_monomial_op!(@internal $op_trait, $op_method, self.clone(), rhs)
}
}

impl Add<&PolynomialBase<$monomial>> for &$monomial {
impl $op_trait<&PolynomialBase<$monomial>> for &$monomial {
type Output = PolynomialBase<$monomial>;
fn add(self, rhs: &PolynomialBase<$monomial>) -> Self::Output {
self.clone() + rhs.clone()
fn $op_method(self, rhs: &PolynomialBase<$monomial>) -> Self::Output {
self.clone().$op_method(rhs.clone())
}
}
};

// Internal helper for optimized Add implementation
(@internal Add, add, $self:expr, $rhs:expr) => {{
let mut rhs = $rhs;
rhs.add_term($self, coeff!(1.0));
rhs
}};

// Internal helper for Sub implementation
(@internal Sub, sub, $self:expr, $rhs:expr) => {{
let mut result = PolynomialBase::from($self);
result -= $rhs;
result
}};
Comment on lines +224 to +235
Copy link

Copilot AI Aug 27, 2025

Choose a reason for hiding this comment

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

[nitpick] The Add and Sub implementations use different approaches: Add mutates the RHS polynomial directly while Sub creates a new polynomial from the monomial. This inconsistency could lead to confusion and different performance characteristics. Consider using a consistent approach for both operations.

Copilot uses AI. Check for mistakes.
}

// Legacy macro for backward compatibility - delegates to generic macro
macro_rules! impl_monomial_add {
($monomial:ty) => {
impl_monomial_op!(Add, add, $monomial);
};
}

// Legacy macro for backward compatibility - delegates to generic macro
macro_rules! impl_monomial_sub {
($monomial:ty) => {
impl_monomial_op!(Sub, sub, $monomial);
};
}
Comment on lines +239 to +250
Copy link

Copilot AI Aug 27, 2025

Choose a reason for hiding this comment

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

[nitpick] The impl_monomial_add and impl_monomial_sub macros are now redundant wrapper macros that only delegate to impl_monomial_op. Consider removing these wrapper macros and calling impl_monomial_op! directly at the call sites to reduce macro indirection.

Copilot uses AI. Check for mistakes.

impl_monomial_sub!(LinearMonomial);
impl_monomial_sub!(QuadraticMonomial);
impl_monomial_sub!(MonomialDyn);

impl_monomial_add!(LinearMonomial);
impl_monomial_add!(QuadraticMonomial);
impl_monomial_add!(MonomialDyn);
Expand Down
Loading