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

Incorrect plotting #15

Open
AayushSabharwal opened this issue Sep 3, 2024 · 7 comments
Open

Incorrect plotting #15

AayushSabharwal opened this issue Sep 3, 2024 · 7 comments

Comments

@AayushSabharwal
Copy link

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

Discrete time and continuous time plots sometimes use the same style

@AayushSabharwal
Copy link
Author

AayushSabharwal commented Sep 5, 2024

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
Copy link
Author

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
Copy link
Author

AayushSabharwal commented Sep 5, 2024

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
Copy link
Collaborator

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

There is something discrete

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

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
Copy link
Author

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
Copy link
Collaborator

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
Copy link
Author

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

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

No branches or pull requests

2 participants