Skip to content

Analyser: fixed an issue with unit cancellation. - #1486

Open
agarny wants to merge 5 commits into
cellml:mainfrom
agarny:issue1485
Open

agarny wants to merge 5 commits into
cellml:mainfrom
agarny:issue1485

Conversation

@agarny

@agarny agarny commented Oct 4, 2026

Copy link
Copy Markdown
Contributor

Fixes #1485.

Copilot AI balanced review requested due to automatic review settings October 4, 2026 21:52

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot review overview

🟡 Changes recommended

The cleanup discards small nonzero exponents before later powers can amplify them, producing incorrect dimensional results.

Review effort: Balanced
Findings: 1 Medium severity

Open (1)
What changed in this PR

Addresses #1485 by removing cancelled unit entries so the analyser recognises dimensionless products regardless of operand order.

Changes:

  • Centralises unit-exponent accumulation and cancellation.
  • Adds regression coverage for three operand orderings.
File Description
tests/​analyser/​analyserunits.cpp Tests dimensionless products across operand orderings.
src/​analyser.cpp Adds and uses the cancellation helper.
src/​analyser_p.h Declares the private helper.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread src/analyser.cpp Outdated

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot review overview

🔵 Needs a closer look

The cancellation check discards legitimate tiny accumulated exponents, causing incorrect units after power operations.

Review effort: Balanced
Findings: None

Resolved since last review (1)
Previously missed (1)

In code that hasn't changed since last review

Medium severity Preserve genuine tiny exponents instead of dropping dimensions

src/​analyser.cpp:1133

This check erases genuine tiny exponents even when nothing cancels. In tinyExponentAmplifiedByPower, adding <unit units="second" exponent="0"/> after the existing 1e-16 entry deletes the second dimension, so the later power appears dimensionless instead of having units of seconds. Reversing those entries preserves the dimension. Two 1e-16 entries also disappear instead of becoming second^2 after the power. Use a cancellation rule that preserves meaningful small accumulated exponents rather than treating every near-zero sum as cancellation, and extend the regression tests to cover zero and same-sign contributions in both orders.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot review overview

🟡 Changes recommended

Numerical edge cases can erase genuine dimensions and incorrectly accept dimensional expressions as dimensionless.

Review effort: Balanced
Findings: 3 High severity

Open (3)

Comment thread src/analyser.cpp Outdated
Comment thread src/analyser.cpp
Comment thread src/analyser.cpp Outdated

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot review overview

🔵 Needs a closer look

The cancellation threshold can erase an exactly representable nonzero exponent and incorrectly classify units as dimensionless.

Review effort: Balanced
Findings: None

Resolved since last review (3)

@agarny
agarny requested review from hsorby and nickerso October 4, 2026 23:09
@agarny

agarny commented Oct 4, 2026

Copy link
Copy Markdown
Contributor Author

@FinbarArgus, this should fix it.

@agarny

agarny commented Oct 4, 2026

Copy link
Copy Markdown
Contributor Author

@FinbarArgus, this should fix it.

I mean, fix issue #1485.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Analyser unit check: a product whose first operand is in units like volt.mV^-1 is reported as not dimensionless; the reordered product passes

2 participants