SciML / ModelingToolkitStandardLibrary.jl

A standard library of components to model the world and beyond
https://docs.sciml.ai/ModelingToolkitStandardLibrary/stable/
MIT License
121 stars 37 forks source link

Rotational: default values are now guesses + fix tests #274

Closed ven-k closed 7 months ago

ven-k commented 7 months ago

Checklist

codecov[bot] commented 7 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 54.46%. Comparing base (13e4e93) to head (3a7ccfb). Report is 8 commits behind head on main.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #274 +/- ## ========================================== + Coverage 0.00% 54.46% +54.46% ========================================== Files 35 46 +11 Lines 1626 1634 +8 ========================================== + Hits 0 890 +890 + Misses 1626 744 -882 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

ven-k commented 7 months ago

@ChrisRackauckas this is ready for review.

ven-k commented 7 months ago

Downgrade CI doesn't seem to be happy yet; but I will recheck up to which versions I can bump them to (as a follow up). I don't want to increase the compats to latest available versions; but keep them just above the minimum required ones.

ven-k commented 7 months ago

The failing Downgrade CI test is fixed by:

baggepinnen commented 7 months ago

Considering how much of the user-facing documentation and tests had to be changed in this PR, I'd say this is a massively breaking change that requires a new major version?


Related, that the user now has to provide initial condition for every variable in every subsystem will make a lot of workflows significantly more unpleasant. For example, dragging components into a GUI and simulating will not be possible without clicking every single component to select the initial condition? Is this really how we want it to behave?

A simple option that uses guesses as defaults if no default is available would be useful to not slow down experimentation and exploration to an absolute grind.


I noticed that defaults were changed to guesses for rotational + two blocks from Blocks. The change makes some sense for the rotational components but not for the blocks. Was there a particular reasons these two blocks and only these were changed?

ven-k commented 7 months ago

I'd say this is a massively breaking change that requires a new major version?

This sounds good. Even the Units PR will be unblocked with this PR. So I'd say we can have that in too. (And probably guesses in other sub-libraries as well - with dedicated PRs for each libraries)

Was there a particular reasons these two blocks and only these were changed?

The idea is - incrementally change all the sub-libraries; tend the ones with broken tests first.

The analysis-point tests were failing as the rotational components now have guesses instead of defaults - fixed with op - while there, I changed these two as they were involved.

I've reverted the changes to Integrator and Derivative blocks.

baggepinnen commented 6 months ago

This was released in a minor version but it should have been a major version bump 😔

ChrisRackauckas commented 6 months ago

It probably should have.