-
-
Notifications
You must be signed in to change notification settings - Fork 160
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
Add theory, intro and use case pages of LRE user guide #2522
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2522 +/- ##
=======================================
Coverage 98.72% 98.72%
=======================================
Files 90 90
Lines 4156 4158 +2
=======================================
+ Hits 4103 4105 +2
Misses 53 53 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Going to suggest @vprusso take the first pass at a review of the material. My comments are small and shouldn't overlap (I hope!).
6f06065
to
2d27879
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great start Purva! I can tell you have a great understanding of this technique, but the this page still needs some work to get all the nuances from your head into the doc so others can understand as well.
9d8e974
to
558ef8e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm still finding this page quite confusing. I like the general structure, but I think many of the explanations are flipped with conclusions coming before explanations, or variables being introduced before they're put into context.
docs/source/guide/lre-5-theory.md
Outdated
O_{\rm LRE} = \sum_{i=1}^M \langle O (\boldsymbol{\lambda}_i)\rangle \frac{\det \left(\mathbf{M}_i (\boldsymbol{0}) \right)}{\det \left(\mathbf{A}\right)}. | ||
$$ | ||
|
||
To get the matrix $\mathbf{M}_i(\mathbf{0})$, replace the $i$-th row of the sample matrix $\mathbf{A}$ by $\mathbf{e}_1=(1, 0, \ldots, 0)^T$ where except $M_1(0, d) = 1$ all the other monomial terms are zero. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Where does this come from?
- Conclusion is needed. Recap what we went through, and maybe add a link to the API-doc LRE section?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where does this come from?
Discussed before the equation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not seeing anything in the linked document about the Lagrange interpolation formula about a matrix or replacing a row with a standard basis vector. If I'm missing something please point out where I'm missing it, but otherwise I think needs a bit more introduction.
Co-authored-by: nate stemen <[email protected]>
Co-authored-by: nate stemen <[email protected]>
* add intro and use case pages Co-Authored-By: Purva Thakre <[email protected]> * clean up intro/use case * clarify depth comment * wordsmithing --------- Co-authored-by: Purva Thakre <[email protected]>
2b45cc9
to
070e348
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for all your back and forth on this Purva! I think we can make some more tweaks to the theory page to make is extra super clear, but lets ship with this and then keep improving.
Description
Fixes #2486
License
Before opening the PR, please ensure you have completed the following where appropriate.