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

Allow separate eom violation plots. replaces PR #273 #310

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Peter230655
Copy link
Contributor

@Peter230655 Peter230655 commented Jan 28, 2025

This allows to see the violations of the equations of motion in detail, each in a separate plot, if the new optional keyword detailed_eoms is set to True. If set to False or not set, the eom violations will be displayed as before.
This replaces PR #273 which does not meet the last test.
I have tried it on many simulations, and at times found substantial differences in the violations of the eoms, say 10^(-8) for the worst to 10^(-18) for the best is no unusual.

@Peter230655 Peter230655 mentioned this pull request Jan 28, 2025
@Peter230655 Peter230655 changed the title replaces PR #273 replaces PR #273 to allow separate eom plots Jan 29, 2025
@Peter230655 Peter230655 changed the title replaces PR #273 to allow separate eom plots Allow separate eom violation plots. replaces PR #273 Jan 29, 2025
@moorepants
Copy link
Member

This puts each equation on a new row:
image

Note that we switched to the single axis deliberately here: #125

@moorepants
Copy link
Member

In general, one is needing to know if any constraints are violated at all. What is the reason to make the default plot show the relative differences in the equation of motion violations?

Secondly , have you tested this with very large # of EoMs? Does it always make a plot that isn't extremely squished vertically?

I prefer that the plot functions just work and have few to no knobs to turn. Anyone can create their own plotting functions or manipulate the returned axes. I'd like these to give a default average plot to quickly view with simple function calls.

@Peter230655
Copy link
Contributor Author

In general, one is needing to know if any constraints are violated at all. What is the reason to make the default plot show the relative differences in the equation of motion violations?

As I remember, with some simulation I wanted to see which eom was violated severely, but I must admit I do not remember exactly. I just thought it does not hurt having it.

Secondly , have you tested this with very large # of EoMs? Does it always make a plot that isn't extremely squished vertically?

I tested it with many of my simulations, and never noticed this. Max number maybe 15 or 20 probably not more.

I prefer that the plot functions just work and have few to no knobs to turn. Anyone can create their own plotting functions or manipulate the returned axes. I'd like these to give a default average plot to quickly view with simple function calls.

The optional detailed_eoms is set to false. So, the knob need not be turned. Then everything is exactly like the current version.

@Peter230655
Copy link
Contributor Author

Peter230655 commented Feb 1, 2025

For info:
The drone example in examples-gallery has 14 eoms.
Detailed printout looks like this:
image

at the bottom it looks like this: (of course the shifting to the left is only because I had to take two screen shots)
image

@Peter230655
Copy link
Contributor Author

Peter230655 commented Feb 1, 2025

This puts each equation on a new row: image

Note that we switched to the single axis deliberately here: #125

This only happens if the eoms should be printed individually, otherwise nothng changes.
What I am saying now is not a proof, just an observation:
Since I made this PR for detailed eoms, I must have run 50 or more simulations with detailed eoms and without - and never had a problem except that in rare cases (I forgot what caused it) the rounding of the errors in the instance constraints did not work. I pushed a simple PR to fix it.

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.

2 participants