Skip to content

Make ModelingToolkit optional #209

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

Closed
wants to merge 15 commits into from
Closed

Make ModelingToolkit optional #209

wants to merge 15 commits into from

Conversation

schillic
Copy link
Contributor

I hope CI runs through (I tested locally in v1.3 and v1.8) and - if so - somebody is willing to merge this and make a new (breaking) release 🙂

Summary

This PR makes ModelingToolkit an optional dependency, which allows to use this package in Julia v1.9 and newer again. The tests still use ModelingToolkit, which is why I downgraded the Julia version in CI. There are also some other minor fixes; in particular, I ran the Aqua tests and now it only still reports piracies and (external) ambiguities.

Note that merging this PR will create clashes with #204, but that work seems to have been abandoned anyway.

Problem description with ModelingToolkit

Only a very old version of ModelingToolkit is supported here (v3). There used to be an issue in Adapt that AdaptStaticArraysExt cannot be compiled. This was fixed upstream, but the fix is inconsistent with the package bounds here. Hence compilation always fails in Julia v1.9 and newer.

Short summary of commits (with the commit messages as names)

  1. .gitignore

Added some files to .gitignore.

  1. fix header spaces in NEWS

Fixed some spacing issues in NEWS.md.

  1. remove redundant version in Project.toml

Removed a redundant version requirement in Project.toml.

  1. remove REQUIRE files

Removed the REQUIRE files.

  1. remove .DS_Store

Removed the DS_Store file.

  1. use isnothing

Use isnothing where applicable.

  1. minor formatting

Minor formatting of files.

  1. remove IntervalRootFinding

Removed the seemingly redundant dependency IntervalRootFinding.

  1. remove undefined exports

Removed undefined exports. This closes #208.

  1. prepare for making ModelingToolkit an optional dependency

This is the biggest change. It adds the dependency Requires (package extensions are not compatible, see description above) and wraps all code relevant to ModelingToolkit in functions that are loaded by Requires. ModelingToolkit is however still a dependency and is still loaded automatically.

  1. make ModelingToolkit an optional dependency

Actually removed ModelingToolkit as a dependency.

  1. use test/Project.toml

Added a test/Project.toml file because the resolver of the package version would not work otherwise. The reason is that, to get the old v3 of ModelingToolkit, the version of StaticArrays must be downgraded.

Note that this will create clashes with #204, but that seems to have been abandoned anyway.

  1. remove unbound arg

Removed a type parameter that was not used.

  1. add coverage badge

Added the coverage badge to the README. I was not sure whether it was omitted on purpose, but the CI script reports the coverage, which does not make sense to me if it is not shown anywhere.

  1. restrict CI to working versions

Restricted the Julia versions used in CI to the lower and upper working versions.

@schillic schillic marked this pull request as ready for review March 11, 2024 20:41
@dpsanders
Copy link
Member

Thanks for doing this!

The package still needs to use an ancient version of ModelingToolkit though, right? Or can it use Symbolics instead?

@schillic
Copy link
Contributor Author

schillic commented Mar 11, 2024

Well, there is #204, but it was not touched in years and I do not know how much was missing. I would say this PR is still a direct step forward and Symbolics could be added later if anybody is willing to do the work - I think I am not :)

EDIT:
Let me add that, with this PR, the package works fine without ModelingToolkit (or Symbolics). These packages just provide an alternative convenient way to define constraints. But this package already offers another interface (e.g., @constraint), which should be fine for most use cases.

@codecov-commenter
Copy link

Codecov Report

Attention: Patch coverage is 82.66667% with 13 lines in your changes are missing coverage. Please review.

Project coverage is 76.23%. Comparing base (4ddec09) to head (3cb92ec).

Files Patch % Lines
src/separator.jl 66.66% 11 Missing ⚠️
src/contractor.jl 91.66% 2 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #209      +/-   ##
==========================================
+ Coverage   74.86%   76.23%   +1.37%     
==========================================
  Files           9       10       +1     
  Lines         541      547       +6     
==========================================
+ Hits          405      417      +12     
+ Misses        136      130       -6     

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

@schillic
Copy link
Contributor Author

Closing this in favor of #210.

@schillic schillic closed this Mar 17, 2024
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.

Removing refine! from export list
3 participants