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.

Sign up for GitHub

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

Jump to bottom

Enable dynamic parameters #238

Open
wants to merge 14 commits into
base: master
Choose a base branch
from
Open

Enable dynamic parameters #238

wants to merge 14 commits into from
+197 −118

Conversation

dsharlet
Copy link
Owner

@dsharlet dsharlet commented Dec 5, 2024 •
edited
Loading

This branch reverts a ~10 year old commit 238ba01 to make simulation parameters (potentiometers) work the way they should: the circuit is compiled with the potentiometer value as a variable, and then the variable is substituted during simulation. This allows continuously and smoothly varying the parameters!

Unfortunately, that approach struggled a lot with performance and scalability. However, some recent discoveries (thanks to conversations with @andy-cytomic and @Federer's #196) have revived this approach! Currently, there's still a pretty significant hit to performance with this branch, but it's not terrible:

Circuit master (kHz) dynamic-parameters (kHz)  speedup (factor)
59 Bassman Preamp+Tone Stack 129.1 82.35 0.6378776143
Big Muff Pi 90.2 54.57 0.6049889135
Boss Super Overdrive SD-1 779.6 440.2 0.5646485377
Bridge Rectifier 812.4 828.1 1.019325455
Common Cathode Triode Amplifier 438.8 426.6 0.9721969006
Common Emitter Transistor Amplifier 1294 1261 0.9744976816
Dunlop Cry Baby GCB-95 308.1 119.9 0.3891593638
Fender 5e3 41.96 21.67 0.5164442326
Ibanez Tube Screamer TS-9 1285 552.4 0.4298832685
Marshall Blues Breaker 521.3 221 0.4239401496
Marshall JCM2000 DSL Preamp 59.91 36.25 0.6050742781
Marshall JCM800 2203 preamp modded 59.13 35.93 0.6076441739
Marshall JCM800 2203 Preamp 61.24 47.49 0.7754735467
MXR Distortion + 522.8 292 0.558530987
MXR Phase 90 38.07 49.92 1.311268716
Op-Amp Model 1482 1085 0.7321187584

The really key observation was from discussing this with @andy-cytomic, I realized that if we make an intermediate variable for capacitor currents, that avoids the really problematic impact of the parameter variables on the solution. More details on #94. This was literally a one line change that already existed in the code! 5398a63 This now means we do something more similar to the standard MNA procedure for capacitors.

Still TODO:

  • Update the VST plugin (cc @mikeoliphant)
  • One of the example circuits is failing on this branch, due to an expression explosion.
  • The performance hit is not great. Unfortunately, the optimization in Major speedup to simulations #237 disproportionately benefited master, so the gap between master and this branch is now even bigger than it otherwise would have been.

I am pretty close to thinking that we should just merge this despite the performance regression. Dynamic parameters are a huge win, I think they're worth the performance hit, and I have a clear line of sight to ComputerAlgebra improvements that should mitigate the regression. I have ideas for how to improve ComputerAlgebra significantly for this new approach. Unfortunately me from 15 years ago was a bit dumb, so it's harder than it should be to fix it.

Fixes #94

dsharlet added 11 commits December 2, 2024 20:14
@dsharlet
Revert "Admitting defeat on solving circuits with pots as variables"
238ba01 
This reverts commit b9f03ca.
@dsharlet
Add a variable to the system for capacitor currents. This seemingly s…
5398a63 
…mall change means that we don't need to solve for differentials before discretization, which may enable dynamic parameters to work! Partially addressing #94
@dsharlet
Don't factor expressions in solve
c271540
@dsharlet
Don't try to parse parameter names as expressions
fc2c9d0
@dsharlet
Refactor sweep min/max for easier experimenting
b187c77
@dsharlet
Remove unused Name
61afff4
@dsharlet
Minor refactor
06af620
@dsharlet
Parameter values don't need to be expressions
ec3c0e3
@dsharlet
Respect Sweep type of Potentiometer/VariableResistor
c01596e
@dsharlet
Don't add unknowns for anonymous constants or other simple expressions
5aa0347
@dsharlet
Merge branch 'master' into dynamic-parameters
0eda7a6
@Federerer
Copy link
Collaborator

Federerer commented Dec 5, 2024 via email

@dsharlet
Copy link
Owner Author

dsharlet commented Dec 5, 2024

Cool, regarding the vst plugin, we can cherry pick some stuff from my implementation.

Thanks for the pointer, I think it will be helpful, I'll take a closer look tomorrow.

Also, did you added the option to disable dynamic parameters for spefific potentiometer?

I didn't, but maybe we should. That would give an escape hatch in the case the performance hit is significant.

I generally was thinking this was necessary when I thought the penalty for dynamic parameters was so much larger. IIRC you said on your branch that only one dynamic parameter was possible at a time. But now, just making all potentiometers dynamic seems to be OK (I can still run a lot of the big amp simulations with all potentiometers dynamic in real time).

I think what we should do is:

  • Merge this branch with the performance penalty, potentiometers always dynamic. Adding an option has a cost in complexity, since we need to support both codepaths.
  • If I can't fix ComputerAlgebra to recover most of the performance loss, we can add an option to make potentiometers dynamic or not before the next release.

@mikeoliphant
Copy link
Collaborator

Looking at this more closely, it probably makes more sense to have @Federerer update the VST, since it will involve refactoring the parameter UI interface.

I thought the UI changes from a while back looked good.

@mikeoliphant
Copy link
Collaborator

mikeoliphant commented Dec 18, 2024 •
edited
Loading

In the interest of moving this along, I took a stab at updating the VST code:

#243

Basically, it just removes the re-build on parameter changes, and passes the parameters when running the simulation.

I'm getting an error when running, though. Not sure why. It happens when using the Mock VST as well, so it's easy to reproduce. @dsharlet any ideas?

@dsharlet
Copy link
Owner Author

Thanks for doing that. I had a similar change I tried locally, and I ran into an issue where it seemed like the lifetime of the Circuit and Analysis objects were somehow not matching. I was hoping I just did something dumb in my VST update, but it sounds like you are hitting the same thing... so I'll take a closer look at that.

BTW, quick update: I've been trying to find quick hack improvements to the computer algebra system to make this mergeable without the issues (performance slowdown + one broken example), but I think it's not happening. I have a clear line of sight to bigger changes to make, so I think I'm still going to merge this soon, and then work on a computer algebra system improvement in a follow-up change.

@mikeoliphant
Copy link
Collaborator

Thanks for doing that. I had a similar change I tried locally, and I ran into an issue where it seemed like the lifetime of the Circuit and Analysis objects were somehow not matching.

I think I've got a race condition in the asyc simulation updating. Looking into fixing it now.

@mikeoliphant
Copy link
Collaborator

I think I've got a race condition in the asyc simulation updating. Looking into fixing it now.

Turns out, no - I forgot the background loading was using a lock.

@mikeoliphant
Copy link
Collaborator

I've been refactoring the LiveSimulation class to break out the underlying simulation run/update behavior so that it can be reused in the VST. I've now got the VST not crashing, but the pots don't work because the values have to be updated differently now. Working on a fix for that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Enable dynamic parameters
3 participants
@dsharlet @Federerer @mikeoliphant