Skip to content

fix pid_lagrange math - #392

Open
AmineAndam04 wants to merge 2 commits into
PKU-Alignment:mainfrom
AmineAndam04:pid_lag
Open

fix pid_lagrange math#392
AmineAndam04 wants to merge 2 commits into
PKU-Alignment:mainfrom
AmineAndam04:pid_lag

Conversation

@AmineAndam04

Copy link
Copy Markdown

Description

The docstring for PIDLagrangian.pid_update states the update rule as:

$\lambda_{t+1} = \lambda_t + (K_p e_p + K_i \int e_p dt + K_d \frac{d e_p}{d t}) \eta$

However, this does not match the actual implementation: there is no learning rate $\eta$ and the update is not incremental ($\lambda_{t+1} = \lambda_t + \dots$).

I suggest to replace the docstring formula with the following:
$\lambda_{t+1} = \max(0,K_p e_p + K_i \int e_p dt + K_d \frac{d e_p}{d t})$

Motivation and Context

Types of changes

What types of changes does your code introduce? Put an x in all the boxes that apply:

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds core functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation (update in the documentation)

Checklist

Go over all the following points, and put an x in all the boxes that apply.
If you are unsure about any of these, don't hesitate to ask. We are here to help!

  • I have read the CONTRIBUTION guide. (required)
  • My change requires a change to the documentation.
  • I have updated the tests accordingly. (required for a bug fix or a new feature)
  • I have updated the documentation accordingly.
  • I have reformatted the code using make format. (required)
  • I have checked the code using make lint. (required)
  • I have ensured make test pass. (required)

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.

1 participant