Skip to content

Analyser unit check raises the multiplier of a unit to its exponent, in contrast to the specification and Units::scalingFactor - #1467

Merged
hsorby merged 6 commits into
cellml:mainfrom
agarny:issue1463
Sep 26, 2026
Merged

hsorby merged 6 commits into
cellml:mainfrom
agarny:issue1463

Conversation

@agarny

@agarny agarny commented Sep 22, 2026

Copy link
Copy Markdown
Contributor

Fixes #1463.

Copilot AI lite review requested due to automatic review settings September 22, 2026 01:17

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

A critical analyser recursion issue can still double-count inherited multipliers.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 1 High severity

Open (1)
What changed in this PR

Fixes unit-scaling behavior so multipliers and prefixes are handled correctly outside unit exponents.

Changes:

  • Corrects unit scaling and analyser multiplier calculations.
  • Adds regression tests and updates expected diagnostics.
File Reviewed changes
tests/​units/​units.cpp Adds scaling-factor regression tests.
tests/​analyser/​analyserunits.cpp Adds analyser regression tests and updates expectations.
src/​units.cpp Corrects custom-unit prefix scaling.
src/​analyser.cpp Corrects analyser multiplier handling.

💡 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

Validator unit-equivalence scaling still uses the outdated traversal and needs to be updated.

Review effort: Lite
Findings: None

Resolved since last review (1)

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

🟢 Approval recommended

All reviewed changes are covered by regression tests with no unresolved blocking issues.

Review effort: Lite
Findings: None

Comment thread tests/units/units.cpp
Comment thread tests/units/units.cpp
Comment thread tests/units/units.cpp Outdated
Indeed, the `unitsMultiplier` parameter is now always zero, so...
@agarny
agarny requested review from hsorby and a lite review from Copilot September 23, 2026 23:15

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

🟢 Approval recommended

The only remaining finding is a minor test-coverage nit that does not block approval.

Review effort: Lite
Findings: None

@hsorby
hsorby merged commit 0249695 into cellml:main Sep 26, 2026
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Analyser unit check raises the multiplier of a unit to its exponent, in contrast to the specification and Units::scalingFactor

4 participants