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

fix: pass VariableUnit of diff var while converting it to a Term #2583

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

Conversation

ven-k
Copy link
Member

@ven-k ven-k commented Mar 28, 2024

Needs:

Units are defined only in MTK, so pass this piece of info to Symbolics.diff2term to add this to the returning term-var.

With this PR: k_t's unit will be 1.0 s⁻¹

using ModelingToolkit
using ModelingToolkit: t, D, get_unit, value, VariableUnit, default_toterm
@variables k(t)
get_unit(default_toterm(D(k))) # returns 1.0 s⁻¹

And prints a clear warning message whenever the term arg has a non SI unit.

julia> @variables l(t) [unit = u"mg"]
julia> l2 = value(D(D(l)))
julia> default_toterm(ModelingToolkit.value(l2))       
┌ Warning: Ignoring the unit while converting `Differential(t)(Differential(t)(l(t)))` to a term.
│ 1.0e-6 kg uses non SI unit. Please use SI unit only. 
└ @ ModelingToolkit C:\Users\Asus\.julia\dev\ModelingToolkit.jl\src\systems\unit_check.jl:176
lˍtt(t)

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

@ven-k ven-k force-pushed the vkb/default-toterm branch 2 times, most recently from 383d591 to 2f9f965 Compare April 12, 2024 07:56
@ven-k ven-k force-pushed the vkb/default-toterm branch 2 times, most recently from 77ecabb to 2809106 Compare May 1, 2024 07:12
ven-k added 4 commits May 1, 2024 21:11
Units are defined only in MTK, so pass this piece of info to `Symbolics.diff2term` to add this to the returning term-var.
    `__get_literal_unit` already screens units of DynamicQuantity.AbstractQuantity. So this check is redundant.
    Also, removing this check fixes the issue with `get_unit` for units of Unitful type.
@ven-k ven-k marked this pull request as ready for review May 1, 2024 17:05
@ven-k
Copy link
Member Author

ven-k commented May 1, 2024

The newly added tests in "Variable Utils Tests" have passed here

As Clock calls default_toterm and the behavior shouldn't change "Parameter Dependency Test" are related too. It has passed here

This PR is ready for review.

Additionally, it removes this accidentally added file mo.diff: 27e2699

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.

1 participant