Skip to content

Add smooth sources in Blocks + Add Square and Triangular sources #59

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

Merged
merged 3 commits into from
Jun 3, 2022

Conversation

ven-k
Copy link
Member

@ven-k ven-k commented May 22, 2022

  • Adds smooth option for sources in Blocks. For Sine, Cosine, ExpSine, Step, Ramp

  • Adds Square and Triangular sources

equation = if smooth == false
offset + ifelse(t < start_time, 0, amplitude* sin(2*pi*frequency*(t - start_time) + phase))
else
δ = 1e-5
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this parameter be exposed and documented?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it should

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps yes. Although having δ = 1e-6 or such makes a miniscule change to the result, it can upset high precision tests

equation = if smooth == false
offset + ifelse(t < start_time, 0, amplitude* sin(2*pi*frequency*(t - start_time) + phase))
else
δ = 1e-5
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it should

end

eqs = [
output.u ~ smooth_square(t, δ, frequency, amplitude, offset, start_time)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there no exact version?

eqs = [
output.u ~ offset + ifelse(t < start_time, 0, amplitude * exp(-damping * (t - start_time)) * sin(2*pi*frequency*(t - start_time) + phase))
output.u ~ smooth_triangular(t, δ, frequency, amplitude, offset, start_time)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there no exact version?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When I had tried the solvers became unstable. Let me give another shot at it

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would leave it in. It is an upstream problem than and not of the library. Also add tests but mark it as broken.

@ven-k
Copy link
Member Author

ven-k commented May 22, 2022

Triangular and Square sources now support start-time and offset.


So these are no longer the waveforms

If Triangular is going to be only smooth, I'm thinking, we should remove start_time and offset options

start_time shifts the wave but value isn't non-zero for t<st (Might have to resort to IfElse)
When offset=0, start_time=1, amplitude=1, f=5

A=1,f=5,st=1,o=0

Offset makes it very erratic
When offset=1, start_time=1, amplitude=1, f=5

A=1,f=5,st=1,o=1

@ven-k
Copy link
Member Author

ven-k commented May 22, 2022


So this is no longer the waveform

Square Wave
square-wave

@ven-k ven-k marked this pull request as draft May 22, 2022 19:08
@ValentinKaisermayer
Copy link
Contributor

Is this an aliasing problem or is the implementation incorrect?

@baggepinnen
Copy link
Contributor

One option that can be provided alongside the smoothing option is to use callbacks to make sure the solver steps exactly at the discontinuities (in value or derivative etc.) of the signal. The symbolic callbacks should be able to handle these signals, but discrete callbacks may be more efficient given that we know the time points of the events beforehand.

@ven-k
Copy link
Member Author

ven-k commented May 29, 2022

For frequency=5, amplitude=1, offset = 2, start_time = 1


Smooth triangular:
smooth-triangular


Triangular
triangular


For frequency=1, amplitude=2, offset=1, start_time=2.5


Smooth Square:
smooth-square


Square:

square

@ValentinKaisermayer
Copy link
Contributor

Cool!
The smooth square and exact square should have the same phase, for the same parameters, though.

@ven-k ven-k marked this pull request as ready for review May 29, 2022 16:08
@ven-k
Copy link
Member Author

ven-k commented May 29, 2022

The smooth square and exact square should have the same phase

Yeah I had noted that. The last commit, I was working on, addresses exactly that. (I've updated the comment above with latest waveform)

@ven-k
Copy link
Member Author

ven-k commented May 29, 2022

One option that can be provided alongside the smoothing option is to use callbacks

I will add this with a follow up PR

@ven-k ven-k mentioned this pull request May 29, 2022
@ven-k
Copy link
Member Author

ven-k commented May 30, 2022

The thermal.jl tests are no longer broken. See the logs here
([email protected] is used)

So can I go ahead & change @test_broken -> @test ?
cc @YingboMa

@ValentinKaisermayer
Copy link
Contributor

If you do, thx. But in a separate PR. 😄

@ven-k
Copy link
Member Author

ven-k commented May 30, 2022

Ok.
Tests of this PR pass. And this is ready

@ChrisRackauckas
Copy link
Member

It looks like tests fail. Change test broken to test.

- For `Sine`, `Cosine`, `ExpSine`, `Step`, `Ramp`

Add Square and Triangular sources

Add a non-zero start_time to smooth Sine, Ramp, ExpSine tests & non-zero `offset` to smooth ExpSine

Add "almost" exact versions of Triangular and Square waves
The formulae from  https://en.wikipedia.org/wiki/Square_wave & https://en.wikipedia.org/wiki/Triangle_wave
 are modified to accomodate any amplitude, frequency, offset and start_time

translate t on x-axis with start_time for square and triangular waves

trigger workflow

Merge branch 'vk/smooth_blocks' of https://github.com/ven-k/ModelingToolkitStandardLibrary.jl into vk/smooth_blocks

thermal tests are no longer broken; so `@test_broken` -> `@test`
@ven-k ven-k force-pushed the vk/smooth_blocks branch from e651f5a to 0f2d737 Compare May 30, 2022 17:30
@codecov
Copy link

codecov bot commented May 30, 2022

Codecov Report

Merging #59 (883baa8) into main (a2d79c9) will increase coverage by 1.65%.
The diff coverage is 98.48%.

@@            Coverage Diff             @@
##             main      #59      +/-   ##
==========================================
+ Coverage   70.12%   71.77%   +1.65%     
==========================================
  Files          23       23              
  Lines         964     1024      +60     
==========================================
+ Hits          676      735      +59     
- Misses        288      289       +1     
Impacted Files Coverage Δ
src/Blocks/sources.jl 98.94% <98.48%> (-1.06%) ⬇️

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

offset=0.0, start_time=0.0, smooth=false)

if smooth
frequency > 25 && @warn "`frequency > 25` can lead to non-triangular wave pattern"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's up with this?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps the smoothing constants need to be frequency dependent?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

f=25, 26, 30

For f>25, the waveform is totally erratic

Perhaps the smoothing constants need to be frequency dependent?

I tried these:

amplitude * (1-2acos((1 - f* δ)sin(2π*(x - start_time)*f))/π)
amplitude * (1-2acos((1 - δ/f)sin(2π*(x - start_time)*f))/π)
amplitude * (1-2acos((1 - δ)sin(2π*(x - start_time)*f*δ))/π)
amplitude * (1-2acos((1 - δ)sin(2π*(x - start_time)*f/δ))/π)
amplitude * (1-2acos(sin(2π*(x - start_time)*f)/π)

None of these relations gave a better result.
I think it comes down to trigonometric functions used. Is there any source I can look at for this?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would imagine that the delta parameter influences how high frequencies can be tolerated?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removing that didn't make any difference (formula 5 above)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The above image is output of just smooth_triangular

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is still open which is why I didn't merge. What's the consensus here? I didn't look into it.

Copy link
Member Author

@ven-k ven-k Jun 3, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aha, the real issue is the stepsize. For <0.001 the plot looks alright. And this is present in both smooth=true/false wave.

for t = 0:0.001:1

tri-smooth-f=26

Should I mention a note on this in docstring?

(Will remove the @warn)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adaptivity should handle it?

Copy link
Member Author

@ven-k ven-k Jun 3, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Somehow it didn't. For offset!=0 similar issue crops up. saveat=0.01 saves. (Here)

Comment on lines 45 to 53
@register_symbolic smooth_cos(x, δ, f, amplitude, ϕ, offset, start_time)
@register_symbolic smooth_damped_sin(x, δ, f, amplitude, damping, ϕ, offset, start_time)
@register_symbolic smooth_ramp(x, δ, height, duration, offset, start_time)
@register_symbolic smooth_sin(x, δ, f, amplitude, ϕ, offset, start_time)
@register_symbolic smooth_square(x, δ, f, amplitude, offset, start_time)
@register_symbolic smooth_step(x, δ, height, offset, start_time)
@register_symbolic smooth_triangular(x, δ, f, amplitude, offset, start_time)
@register_symbolic triangular(x, f, amplitude, offset, start_time)
@register_symbolic square(x, f, amplitude, offset, start_time)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why register these?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Earlier we didn't assign parameters while declaring them inside components. So @register was necessary.

But yeah, now its no longer required. Removed

@ven-k ven-k requested a review from ChrisRackauckas June 3, 2022 07:32
@ChrisRackauckas ChrisRackauckas merged commit 9a5d70d into SciML:main Jun 3, 2022
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.

4 participants