JuliaComputing / ModelingToolkitSampledData.jl

Other
8 stars 1 forks source link

Incorrect plotting #15

Open AayushSabharwal opened 2 months ago

AayushSabharwal commented 2 months ago

https://juliacomputing.github.io/ModelingToolkitSampledData.jl/dev/tutorials/noise/#Quantization

Discrete time and continuous time plots sometimes use the same style

AayushSabharwal commented 2 months ago

Turns out this is a very specific and annoying scenario. JSC simplifies this system to the extent that there are no discrete variables: m.quant.y is just an observed quantity. It doesn't even have a clock runtime, so as per the current implementation it's a purely continuous system.

AayushSabharwal commented 2 months ago

Looking at the component definition, there isn't any discrete-ness. It gets a continuous input, and gives a continuous output. Discrete subsystems are "delimited" by specific operators: any input is a sample, any output is a hold.

AayushSabharwal commented 2 months ago

Consider the following modification of the example:

julia> @mtkmodel QuantizationModel begin
           @components begin
               input = Sine(amplitude=1.5, frequency=1)
               quant = Quantization(; z, bits=2, y_min = -1, y_max = 1)
               quant_c = Quantization(; z, bits=2, y_min = -1, y_max = 1)
           end
           @variables begin
               sine_disc(t) = 0
               x(t) = 0 # Dummy variable to work around a bug for models without continuous-time state
           end
           @equations begin
               sine_disc ~ Sample(Clock(0.1))(input.output.u)
               quant.input.u ~ sine_disc
               connect(input.output, quant_c.input)
               D(x) ~ 0 # Dummy equation
           end
       end
julia> @named m = QuantizationModel()
julia> m = complete(m)
julia> ssys = structural_simplify(IRSystem(m))
julia> prob = ODEProblem(ssys, [], (0.0, 2.0))
julia> sol = solve(prob, Tsit5())
julia> plot(sol, idxs=[m.input.output.u, m.quant.y, m.quant_c.y, m.sine_disc])

image

Note the difference between the continuous and discrete quantization.

Quantization (as implemented) itself does not make a variable discrete. It is a transformation of an input signal to an output signal. If you want it to take a continuous input and give a discrete output, you'd need to throw a Sample in there.

baggepinnen commented 2 months ago

Looking at the component definition, there isn't any discrete-ness.

There is something discrete https://github.com/JuliaComputing/ModelingToolkitSampledData.jl/blob/5947921f3ee9010cf353ed9b62c34415f6d5d631/src/discrete_blocks.jl#L981

the variables are being indexed by the shift index

y(z) ~ ifelse(quantized == true, quantize(u(z), bits, y_min, y_max, midrise), u(z))

this should be enough for JSC to determine that the entire system is discrete. Shifts are used in both input_timedomain, output_timedomain as holding time-domain information, e.g.,

function ModelingToolkit.input_timedomain(x::IRElement)
    @match x begin
        Term(&DT, _...) || Term(&SAMPLE, _...) => SciMLBase.Continuous
        Term(&HOLD, _...) || Term(&SHIFT, _...) => ModelingToolkit.InferredDiscrete
        Term(&CLOCKCHANGE, [_, Const((from, _), _), _...], _...) => SciMLBase.Clock(from)
        Term(_...) => error("$x is not an operator.")
        _ => nothing
    end
end
AayushSabharwal commented 2 months ago

y(z) is not a shifted term, it just sets the VariableTimeDomain metadata to that of the shift. The problem is that (as I understand it) clock inference will see that the input to the quantization is connected to a continuous signal (the sine wave) and thus make the entire subsystem continuous. It can't make the entire subsystem discrete, because this would imply that the independent variable t is discrete.

baggepinnen commented 2 months ago

t is neither continuous nor discrete, or rather, it can be both. It does not imply any time domain, in the current implementation. We have it this way so that one does not have to sample trivial mathematical functions and can instead just write them by referring to time. Indexing with z should imply a time domain though, if it doesn't at the moment, I'd say that this is the bug.

it just sets the VariableTimeDomain metadata to that of the shift

that sounds good, would it then be a matter of adding a check for this VariableTimeDomain in the case Term(&HOLD, _...) || Term(&SHIFT, _...) => ModelingToolkit.InferredDiscrete?

AayushSabharwal commented 2 months ago

I see, thanks for the clarification. The check would have to be in extract_ir, where the Symbolics variables are converted to JSC IRElements.