Skip to content
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.

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

Replace Expronicon with Moshi #922

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from
Draft

Replace Expronicon with Moshi #922

wants to merge 2 commits into from

Conversation

visr
Copy link
Contributor

@visr visr commented Jan 24, 2025

This is used for the exported Algebraic Data Type (ADT) TimeDomain.

Closes #741 which wasn't merged due to invalidations caused by newer Expronicon releases. Moshi is the replacement by the same author that fixes this issue. It is a different design so some care is taken to minimize disruption.

One benefit of this change is that it removes these dependencies from SciMLBase: Pkg, Expronicon, MLStyle, ArgTools, Downloads, FileWatching, InteractiveUtils, LibCURL, REPL, Tar, LibCURL_jll, MozillaCACerts_jll, Zlib_jll, nghttp2_jll, p7zip_jll

Moshi only depends on ExproniconLite and Jieko.

One of the differences between Expronicon and Moshi is that the former ADT macro creates a type, whereas the latter creates a module with the Type in there. Since the ADT was manually placed in the Clocks module, I could just rename the ADT to the module Clocks, and the old ADT name is kept as const TimeDomain = Clocks.Type. With that TimeDomain can still be used for dispatch or container typing like is done in ModelingToolkit.

The DiscriminatorType code was only added for Clock serialization in #753, but that now works without extra code.

The biggest change that I couldn't work around is that Continuous and SolverStepClock, both singleton variants, should now be explicitly constructed with Continuous instead of Continuous(). My idea was to land and release #921 first, so that ModelingToolkit can update to this new syntax, and then merge this next. Though if you prefer to just update both SciMLBase and ModelingToolkit together then #921 wouldn't be needed.

EDIT: This is a draft because this will break MTK unless SciML/ModelingToolkit.jl#3353 is merged and released first.

@ChrisRackauckas
Copy link
Member

My idea was to land and release #921 first, so that ModelingToolkit can update to this new syntax, and then merge this next. Though if you prefer to just update both SciMLBase and ModelingToolkit together then #921 wouldn't be needed.

It's not just MTK but also SymbolicUtils.jl that needs it, and it's been a high priority thing to get that update in some time.

But yes let's do this in steps.

@visr visr marked this pull request as draft January 27, 2025 09:53
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.

2 participants