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

Create independent variables with @independent_variables #2862

Merged
merged 21 commits into from
Jul 17, 2024

Conversation

hersle
Copy link
Contributor

@hersle hersle commented Jul 16, 2024

I suggest this to close #2818 and prevent others from making the same mistake.

This checks that independent variables are @parameters, to be consistent with t_nounits.

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

@isaacsas
Copy link
Member

isaacsas commented Jul 16, 2024

This would be a breaking change though? For years MTK had made t a variable and such warnings/errors were not included in MTK9 so users would’ve assumed it was still possible.

@hersle
Copy link
Contributor Author

hersle commented Jul 16, 2024

I suppose, I think some tests will fail without modification.

The alternative, of course, is to require the independent variable to be defined with @variables instead of @parameters.
Or both definitions should be supported throughout.

Just a suggestion for now. Opinions are welcome :)

@isaacsas
Copy link
Member

I don’t have an opinion on it either way; MTK has bounced between using parameters vs variables for ivs multiples times now, but if this is merged it should only be released within an MTK10 release since it seems like a breaking change.

@ChrisRackauckas
Copy link
Member

We already made the breaking change with v9 that everything documented says to use the global time anyways.

using ModelingToolkit: t_nounits as t, D_nounits as D

it's in every tutorial and it's in the upgrade guide.

@ChrisRackauckas
Copy link
Member

You need to run the formatter for tests to run.

using JuliaFormatter
format(raw"C:\Users\accou\.julia\dev\ModelingToolkit/") # or whatever the path is

@isaacsas
Copy link
Member

isaacsas commented Jul 16, 2024

The ODESystem docstring itself still uses @variables.

@isaacsas
Copy link
Member

isaacsas commented Jul 16, 2024

And I thought the migration guide just said that the defaults should be used, not that ivs now need to be parameters?

@ChrisRackauckas
Copy link
Member

Well we should definitely update any docs that were missed to import the t from the package. We probably missed a few since it was a huge change, that's not intended.

But ultimately, if structural simplify needs it in one form or the other, that determines it. It really didn't seem to matter before, but maybe @YingboMa can comment on why it matters here.

@ChrisRackauckas
Copy link
Member

oh actually I see what's going on. This is a byproduct of initialization, since when initialization is running it has to be treated as a parameter and during that process the structural simplification cares. This means this bug only effects DAEs with non-trivial initialization problems where the non-trivial subset is non-autonomous, i.e. the initialization system is required and an initialization equation is directly a function of t. It seems fairly expensive to support it not being a parameter then, so that settles the debate once and for all.

But I do want to check in on something. A seemingly unrelated error SciML/SciMLBenchmarks.jl#1007 in dummy derivative sorting in initialization seems suspiciously related.

@hersle
Copy link
Contributor Author

hersle commented Jul 16, 2024

This means this bug only effects DAEs with non-trivial initialization problems where the non-trivial subset is non-autonomous, i.e. the initialization system is required and an initialization equation is directly a function of t.

Exactly. #2818 is indeed a very simple such example (although with a trivial linear initialization equation).

@isaacsas
Copy link
Member

If such a change is now being considered, would it perhaps make sense to add metadata to tag an iv as an actual iv? This seems useful anyways...

@isaacsas
Copy link
Member

So an iv could now be a parameter with specific metadata indicating it is an iv?

@isaacsas
Copy link
Member

(It just seems like this back and forth between releases switching ivs from parameters to variables and now back to parameters is indicative that they aren't really either, and so need some more specific way to be flagged internally.)

@hersle
Copy link
Contributor Author

hersle commented Jul 16, 2024

It is also confusing that the independent variable is a parameter (e.g. #564).
The literature already mixes the terms independent variable and independent parameter, though.

@hersle
Copy link
Contributor Author

hersle commented Jul 16, 2024

I just "pushed through" on this to see that everything can work as expected with the change.

@hersle
Copy link
Contributor Author

hersle commented Jul 16, 2024

We could also export a macro like @independent_variables t that (for now) just returns @parameters t, but can be changed internally later with smaller chances of breaking user code.

@isaacsas
Copy link
Member

Yes, I think something like that would be a good approach. Though adding metadata at some point would be useful because then a function like isiv(sym) would be trivial to implement (and useful to have).

@ChrisRackauckas
Copy link
Member

We could also export a macro like @independent_variables t that (for now) just returns @parameters t, but can be changed internally later with smaller chances of breaking user code.

I think that would be good. And to address Sam's concern of being a breaking change, we can change this to a deprecation warning that the independent variables should be defined with @independent_variables and this will error in future versions.

@hersle hersle changed the title Check that independent variables are defined as @parameters Create independent variables with @independent_variables Jul 17, 2024
@hersle
Copy link
Contributor Author

hersle commented Jul 17, 2024

I have added an @independent_variable t macro. Now it just wraps @parameters t, though, so they are both allowed and fully equivalent.

I guess it is possible to treat independent variables as a separate "variable type", but it requires even more work. With a separate treatment, I suppose check_independent_variables() could issue a warning if the independent variable is created with @parameters or @variables.

What do you think?

@hersle
Copy link
Contributor Author

hersle commented Jul 17, 2024

Should

@variables x y(x)
@named sys = ODESystem([y ~ 0], x)

just issue a warning, instead of failing hard?

@ChrisRackauckas
Copy link
Member

just issue a warning, instead of failing hard?

To be technically non-breaking, yes.

I guess it is possible to treat independent variables as a separate "variable type", but it requires even more work. With a separate treatment, I suppose check_independent_variables() could issue a warning if the independent variable is created with @parameters or @variables.

I think that's fine. We can document everything to use the new macro for now, and then when we get more sophisticated add a warning to just @parameters down the line. So it's soft deprecated now, but can do the further deprecation when we find a reason to change the internals to distinguish the independent variables more. Even when that happens, we would still want to label them as parameters for the reason above, so it would just be more metadata added.

@ChrisRackauckas
Copy link
Member

Wait, not fully approved because error to warn.

@ChrisRackauckas ChrisRackauckas merged commit 8126cd9 into SciML:master Jul 17, 2024
21 of 22 checks passed
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.

Custom independent variable leads to underdetermined initialization system
3 participants