Skip to content

Conversation

@cmhyett
Copy link
Contributor

@cmhyett cmhyett commented Dec 2, 2025

Checklist

  • Appropriate tests were added
  • Any code changes were done in a way that does not break public API
  • All documentation related to code changes were updated
  • The new code follows the
    contributor guidelines, in particular the SciML Style Guide and
    COLPRAC.
  • Any new documentation only uses public API

Additional context

Should close #3842, albeit with a solution for a different problem than stated there originally.

@cmhyett
Copy link
Contributor Author

cmhyett commented Dec 3, 2025

Unit checking logic is duplicated between src/systems/unit_check.jl and src/systems/validation.jl I'm not sure if this is intended or not. It seems unit_check.jl and validation.jl are perhaps used in different instances?

Regardless, the logic for the fix is simple

  • Previously MTK was trying to check units on an AnalysisPoint, but didn't have a method defined.
  • AnalysisPoints can be thought of as just connections with additional data so I added a simple passthrough
    • When checking units on an AP, remake the connection without the AP and use existing logic to check the units
  • Added a test to check this functionality is hit. (The test actually calls the validation.jl version, again, unsure if the unit_check.jl is ever used, but seemed prudent to include the logic there as well).

@ChrisRackauckas ready for your review; I believe the test failures are coming from the main branch, please advise if you'd like me to rebase this on an upstream version.

@cmhyett cmhyett marked this pull request as ready for review December 3, 2025 17:18
# no units first
@mtkmodel FirstOrderTest begin
@components begin
in = Blocks.Step()
Copy link
Member

Choose a reason for hiding this comment

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

You'll need to import MTKStdlib.Blocks for this test. It's currently failing in CI.

Copy link
Contributor Author

@cmhyett cmhyett Dec 4, 2025

Choose a reason for hiding this comment

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

Sure, I think the submodule is already loaded (twice in fact!), so I just removed the Blocks namespacing. I'll keep an eye on the tests and ping this when it is resolved.

Submodule already loaded.

@testset "AnalysisPoint is lowered to `connect`" begin
@named P = FirstOrder(k = 1, T = 1)
@named C = Gain(; k = -1)
Copy link
Contributor Author

@cmhyett cmhyett Dec 4, 2025

Choose a reason for hiding this comment

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

CI test failure is pointing here (and other uses of Gain below, not the testsets I added above); Gain is exported by two modules, MTKStdLib.Blocks and...something else? and the ambiguity is causing the error. Unsure why it's throwing after my changes, but let me see if I can specify the module to fix.

Copy link
Member

@AayushSabharwal AayushSabharwal left a comment

Choose a reason for hiding this comment

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

Thanks!

@AayushSabharwal AayushSabharwal merged commit 86a3a53 into SciML:master Dec 4, 2025
29 of 51 checks passed
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.

erroneous cycles started appearing in simple systems

2 participants