add toplevel arguments to various getters

[TorkelE]

Needs tests and docs, and to check if there are any other getters that should have a similar arguments.

But figured I'd upload this first in case people had input/opinions about the approach.

[TorkelE]

I think this is a leftover from the code being copied from the observed getter (?), in which case it would make sense to use cbs and not obs throughout the function.

[AayushSabharwal]

If the system is flattened (or completed), this approach won't work since e.g. get_unknowns contains all the unknowns of the system. This can be handled by doing e.g.

function top_level_unknowns(sys::AbstractSystem)
    if has_parent(sys) && (p = get_parent(sys)) !== nothing
        return top_level_unknowns(p)
    end
    return get_unknowns(sys)
end

[TorkelE]

Was mostly really thinking of an modified accessor of the actual field. However, handling flattened systems like this does make lots of sense (as I think otherwise the §toplevel§ argument would be irrelevant anyway).

Will modify, thanks for the code snippet!

[TorkelE]

@AayushSabharwal For systems where I have thrown a structural simplify I am a bit unsure how to adapt your approach.

I.e. if you want to do observed(sys; top-level = true) on a system which got observables due to structural simplify, going back to the original parent seem to go back to the pre-ss parent, which does not have an observable.

[AayushSabharwal]

Yes. As we discussed in an earlier thread, observed equations are a result of structural simplification which does not retain the hierarchical structure. They don't exist in the hierarchical system.

[TorkelE]

So would it make sense to just disable toplevel for observables?

There is a slightly similar phenomenon for equations, where equations(sys; top-level = false) and equations(sys; top-level = true) give different results. I.e. since accesses the parent parent (and so on) system, it will return equations that was eliminated as observables by structural_simplify. Meanwhile, equations(sys; toplevel = false) will not returns these. It feels wrong that adding top-level = true might cause the addition of equations to the output.

(generally, I do think support for some form of non-ss observables is important, as the current approach kind of skrew up jump simulations hard, but that is a discussion for another place)

[AayushSabharwal]

So would it make sense to just disable toplevel for observables?

Yes. And yeah equations basically suffer the same fate. They don't "belong" to a subsystem anymore.

[TorkelE]

Got it, I will remove the toplevel option for these then. Alternatively, I could have it error for non-complete systems or something like that? It would be useful to have access to these n as many cases as possible.

It would make it possible to still use these when constructing things of MTK. I.e. a problem with a lot of things now being considered extra MTK internal is that while this makes sense for direct users, packages like Catalyst that builds on MTK kind of need these kind of things to construct the models we do.

[AayushSabharwal]

@ChrisRackauckas should we allow passing observed for non-structurally-simplified systems as experimental functionality? This would allow Catalyst to eliminate variables.

@TorkelE In general with simplification we can't allow user-provided observed equations. At best during simplification we club them together with the rest of the equations but this won't guarantee they're re-eliminated, so there really isn't a benefit to not putting them with the rest of the equations.

[AayushSabharwal]

Got it, I will remove the toplevel option for these then. Alternatively, I could have it error for non-complete systems or something like that? It would be useful to have access to these n as many cases as possible.

For observed toplevel doesn't make sense. For equations, you could keep it if it errors for complete systems (why would it error for non-complete systems?). I also don't fully understand why you need these but it isn't difficult to maintain so we might as well. Can we separate them out as a different set of functions? The keyword effectively completely changes the meaning of the function so I don't think it makes sense to club them together.

[TorkelE]

Makes sense. Maybe at some point we try to get as many of us as possible together and discuss through what things Catalyst needs/don't need and the best way to get it from MTK.

This one is finished sans:

• Some minor documentation.
• EDIT: Aayush answered this one already while I was writing.

[TorkelE]

Got it, I will remove the toplevel option for these then. Alternatively, I could have it error for non-complete systems or something like that? It would be useful to have access to these n as many cases as possible.

For observed toplevel doesn't make sense. For equations, you could keep it if it errors for complete systems (why would it error for non-complete systems?). I also don't fully understand why you need these but it isn't difficult to maintain so we might as well. Can we separate them out as a different set of functions? The keyword effectively completely changes the meaning of the function so I don't think it makes sense to club them together.

The reason that I ideally would like to keep it as a kwarg is that it means that I don't have to copy lots of code over, and if someone does something to the main function (i.e. exception for a new MTK-added parameter), those changes will automatically get incorporated without someone having to keep track of these extra functions.

[AayushSabharwal]

if you guys think that makes sense.

It only makes sense for un-flattened systems. But whether the function itself is useful depends on what you need it for. MTK itself (and other uses I'm aware of) don't really need this functionality.

Maybe at some point we try to get as many of us as possible together and discuss through what things Catalyst needs/don't need and the best way to get it from MTK.

Then shouldn't we wait before merging this PR? Adding features we later might deem unnecessary only adds to the maintenance burden and restricts semver.

The reason that I ideally would like to keep it as a kwarg is that it means that I don't have to copy lots of code over, and if someone does something to the main function (i.e. exception for a new MTK-added parameter), those changes will automatically get incorporated without someone having to keep track of these extra functions.

I'm not sure what you mean by "copy code over"? What code needs to be copied? The snippet I sent above handles everything, since it extracts information from the user-provided unsimplified model and thus doesn't have any extra parameters MTK adds.

[TorkelE]

It only makes sense for un-flattened systems. But whether the function itself is useful depends on what you need it for. MTK itself (and other uses I'm aware of) don't really need this functionality.

Right now we use get_eqs quite frequently when building ReactionSystems. However, since it does not have he issues get_ps have I haven't had to look at these cases closer. However, it would be nice to at least have something future proofing ourselves.

Then shouldn't we wait before merging this PR? Adding features we later might deem unnecessary only adds to the maintenance burden and restricts semver.

I was talking in regards to observed. I agree that right now we should remove that part of the PR.

The reason that I ideally would like to keep it as a kwarg is that it means that I don't have to copy lots of code over, and if someone does something to the main function (i.e. exception for a new MTK-added parameter), those changes will automatically get incorporated without someone having to keep track of these extra functions.

So your code uses get_unknowns(sys) which returns the content of the field, which means that we are creating something that the user potentially shouldn't use (but maybe I am still confused). I.e. for parameters the field contain all of those initialisation parameters, and I am trying to recreate the functionality of e.g. parameters but for only the top-level system. So I need it to go through the bit in parameters where you filter this kind of stuff out.

[AayushSabharwal]

for parameters the field contain all of those initialisation parameters, and I am trying to recreate the functionality of e.g. parameters but for only the top-level system. So I need it to go through the bit in parameters where you filter this kind of stuff out.

That's exactly what I'm referring to. These initialization parameters are added to the simplified/completed system and not to the user-provided system. In general, all operations on systems are out-of-place. This is why the approach in my snippet won't have any extra parameters.

[AayushSabharwal]

Right now we use get_eqs quite frequently when building ReactionSystems.

Why? And do you do it on completed systems or unmodified ones?

[TorkelE]

That's exactly what I'm referring to. These initialization parameters are added to the simplified/completed system and not to the user-provided system. In general, all operations on systems are out-of-place. This is why the approach in my snippet won't have any extra parameters.

Ok, got it, will try this out and see if it works. Does that mean that get_ and similar are still permitted for non-complete systems?

Why? And do you do it on completed systems or unmodified ones?

Different in different cases.

[AayushSabharwal]

Does that mean that get_ and similar are still permitted for non-complete systems?

Yep, it's a field accessor. Works on all systems. We use it in a bunch of places.

[TorkelE]

Yep, it's a field accessor. Works on all systems. We use it in a bunch of places.

Got it.

[TorkelE]

Ok, this one should be ready now.

[TorkelE]

I have checked the spell check, and its marked errors are not related to this PR. The other errors also seem to also occur on other branches. It would be good if @AayushSabharwal could have a look to check that things looks sound though.

[AayushSabharwal]

Why is this broken?

[TorkelE]

Because #3464 makes parameters return an internal parameter DEF.

[AayushSabharwal]

The fix for that just got merged and tagged. Could you rebase and try?

[TorkelE]

will do

[TorkelE]

Ok, this is ready now, again, the errors is things which seems to error on other branches (or at least they error on #3466 as well)

[ChrisRackauckas]

Add the docstrings to the doc?

[TorkelE]

Do you have a suggestion on where to put them? There is not dedicated API list in MTK right?

Right now I just added mentions of these, but am happy to insert the docstrings if there are some appropriate positions for them.

[ChrisRackauckas]

Forgot we hadn't updated this repo yet for the newer Documenter features.

[TorkelE]

Right! Looking forwards to checking those out!