feat: add analysis points #3285
Conversation
AayushSabharwal
force-pushed
the
as/analysis-points
branch
from
71095f1 to
9af0038
Compare
test/analysis_points.jl
Outdated
| (nameof(inputap), [nameof(outputap)]) | ||
| ] | ||
| if input isa Symbol | ||
| # broken because MTKStdlib defines the method for |
There was a problem hiding this comment.
Can this method not be moved to MTK?
There was a problem hiding this comment.
Will look into disabling the method in Stdlib if this one is defined in MTK
| each level in the hierarchy is given by elements of `hierarchy`. For example, if | ||
| `hierarchy = [:a, :b, :c]`, the system being searched for will be `root.a.b.c`. Note that | ||
| the hierarchy may include the name of the root system, in which the first element will be | ||
| ignored. For example, `hierarchy = [:root, :a, :b, :c]` also searches for `root.a.b.c`. |
There was a problem hiding this comment.
will this be problematic if the system contains root.root.a.b.c? How do we know whether one root should be removed or not?
There was a problem hiding this comment.
Yeah that is a bit of a problem. The main issue here is that with getproperty syntax to access analysis points, the returned analysis point may or may not have the namespace of the outermost system depending on whether the outermost system is marked with complete or not. We have to handle both cases, but it leads to ambiguities. In your example, the best way to do it would be to ensure the outermost root is not marked as complete. I'm open to better suggestions.
There was a problem hiding this comment.
I think it has to follow the same convention as for variable access, otherwise it'll be confusing. This is why I was using complete everywhere, and why I was sad to see complete change behavior since it broke my code
src/systems/analysis_points.jl
Outdated
|
|
||
| function apply_transformation(cst::ComplementarySensitivityTransform, sys::AbstractSystem) | ||
| sys, (u,) = apply_transformation(GetInput(cst.ap), sys) | ||
| sys, (du,) = apply_transformation(AddVariable(cst.ap, Symbol(:comp_sens_du)), sys) |
There was a problem hiding this comment.
| sys, (du,) = apply_transformation(AddVariable(cst.ap, Symbol(:comp_sens_du)), sys) | |
| sys, (du,) = apply_transformation(AddVariable(cst.ap, :comp_sens_du), sys) |
?
There was a problem hiding this comment.
This is fixed now. Each of these added variables has a name derived from the analysis point to avoid issues where get_comp_sensitivity is called with multiple analysis points in the same (sub-)system.
AayushSabharwal
force-pushed
the
as/analysis-points
branch
from
9af0038 to
b6a1ab8
Compare
|
I'd really appreciate feedback on the docstrings, and suggestions for the docstring for the new |
|
There are lots of docstrings from the old implementation missing, all these + the ones for |
|
Should we make this new interface experimental as well? |
we could, but ideally not for too long. We can mark it as stable once we have integrated with JSML and made sure that works? |
|
I'm not advocating for it being experimental, so it would be great to have this be stable if you're happy with it. |
…ar analysis functions
AayushSabharwal
force-pushed
the
as/analysis-points
branch
from
8bc61de to
f7da682
Compare
If you feel like it should, I'm fine with that as well |
|
Should we add some version of this warning? |
|
While that check can be implemented in MTK, the concept of a |
|
Yeah this would act on the IO metadata, and it only works in setting that have those properties anyways (regardless of the metadata), so that's likely fine. If the user commits the mistake the warnings warn against, the result will be silently wrong, so we have to either warn or error. |
Checklist
contributor guidelines, in particular the SciML Style Guide and
COLPRAC.
Additional context
Add any other context about the problem here.