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

Add ModelingToolkit extension #61

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

Conversation

StevenWhitaker
Copy link

This PR includes the following changes:

  • Adds an extension StateSpaceEconMTKExt that enables creating ModelingToolkit systems from ModelBaseEcon models.
  • Adds a module MTKExt that declares 0-method functions that will have methods added to them in the extension. (This is because extensions can't add new functions to the parent package; extensions can only add methods to existing functions in the parent package.)
  • Widens the method signature of several methods in src/stackedtime/solverdata.jl to allow automatic differentiation to work.
  • Adds some tests for the extension.

Note that there are a couple of (hopefully minor) TODOs in ext/StateSpaceEconMTKExt that might be good to get some feedback on.

@bbejanov
Copy link
Member

Could you please see why it fails to install under Julia 1.9? It's okay for the extension to work only under 1.10, but ideally people should be able to continue to use StateSpaceEcon in earlier versions of Julia albeit without the extension.

Also, please add 1.10 in .github/workflows/main.yml, so that the tests can run. We can drop support for 1.7 and 1.8, but we must keep 1.9 at least for a little while longer.

@StevenWhitaker
Copy link
Author

It appears ModelingToolkit v9 requires Julia 1.10, which is why the test environment can't resolve in Julia 1.9. However, StateSpaceEcon can still install and works fine in Julia 1.9. Ideally, ModelingToolkit should be a test dependency only in Julia 1.10, but I'm not sure if there's a way to have different test environments for different Julia versions. I will check with @bolognam and @bradcarman when they are back from traveling. Another option would be to split this extension out into a separate package.

@codecov-commenter
Copy link

codecov-commenter commented Jul 18, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.76%. Comparing base (1744542) to head (3266ddd).

Additional details and impacted files
@@            Coverage Diff             @@
##           master      #61      +/-   ##
==========================================
- Coverage   89.10%   88.76%   -0.34%     
==========================================
  Files          31       32       +1     
  Lines        2358     2447      +89     
==========================================
+ Hits         2101     2172      +71     
- Misses        257      275      +18     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@bbejanov
Copy link
Member

Thanks for clarifying this - I understand how it works now.

I was able run the 1.9 tests with the following change - keep the default test environment to not include MTK, so it can run in older versions, and Pkg.add the extra packages and include the mtk tests, but only if running 1.10+. Maybe @bradcarman and @bolognam will know a more elegant solution.

diff --git a/Project.toml b/Project.toml
index 61e7646..3a1f68e 100644
--- a/Project.toml
+++ b/Project.toml
@@ -47,7 +47,8 @@ ModelingToolkit = "961ee093-0014-501f-94e3-6117800e7a78"
 NonlinearSolve = "8913a72c-1f9b-4ce2-8d82-65094dcecaec"
 Suppressor = "fd094767-a336-5f1f-9728-57cf17d0bbfb"
 SymbolicIndexingInterface = "2efcf032-c050-4f8e-a9bb-153293bab1f5"
+Pkg = "44cfe95a-1eb2-52ea-b672-e2afdf69b78f"
 Test = "8dfed614-e22c-5e08-85e1-65c5234f0b40"

 [targets]
-test = ["Test", "ModelingToolkit", "SymbolicIndexingInterface", "NonlinearSolve"]
+test = ["Test", "Pkg"]
diff --git a/test/runtests.jl b/test/runtests.jl
index cb4a793..d4fd61d 100644
--- a/test/runtests.jl
+++ b/test/runtests.jl
@@ -409,7 +409,11 @@ end

 include("sim_solver.jl")

-include("mtkext.jl")
+if VERSION >= v"1.10"
+    using Pkg
+    Pkg.add(["ModelingToolkit", "SymbolicIndexingInterface", "NonlinearSolve"])
+    include("mtkext.jl")
+end

 # keep this one last because it overwrites getE?()
 include("modelchanges.jl")

@bradcarman
Copy link

Where does the requirement for Julia v1.10 come from? If I look at ModelingToolkit.jl Project.toml the compat for Julia is v1.9. Is there another dependancy that is triggering Julia v1.10?

@StevenWhitaker
Copy link
Author

You are correct, MTK lists Julia 1.9. However, there are at least three compatibility restrictions from dependencies of MTK:

  1. MTK 9.20 requires NonlinearSolve 3.12. We need MTK 9.20 for the structural_simplify conservative option, but NonlinearSolve requires Julia 1.10 starting from v3.5.0.
  2. MTK 9.20 requires SciMLBase 2.28, which requires Julia 1.10 (starting from v2.25.0).
  3. MTK 9 requires at least Symbolics v5.20.1, which requires Julia 1.10 (starting from v5.15.0).

@bradcarman
Copy link

I think to get this right we need to setup the extension test differently. See here: https://github.com/SciML/ModelingToolkit.jl/tree/master/test/extensions for an example

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.

4 participants