-
Notifications
You must be signed in to change notification settings - Fork 91
Barnett Zehnwirth documentation and formulation rework #657
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
base: master
Are you sure you want to change the base?
Conversation
…th implementation. Added note about difference in formulation. Fixed valuation (iota) summation term.
…ple from 2008 paper. Updated reference list
…. More changes to BZ documentation.
…raphs from 2008 paper for 'reasonable model'
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #657 +/- ##
=======================================
Coverage 85.05% 85.06%
=======================================
Files 85 85
Lines 4866 4868 +2
Branches 619 621 +2
=======================================
+ Hits 4139 4141 +2
Misses 518 518
Partials 209 209
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
I would consider building it directly into the PatsyFormula class in this package
List would be cleaner.
Large changes to the doc is impossible to review at the moment outside of downloading the notebook. Can you please either create a RTD instance on your fork or enable doc versioning on the main repo? |
Shouldn't be too hard, but I'll have to add some logic to handle BZ being passed a formula and Greeks. I'll probably have it throw an error if both are given, since the coefficients produced by basic Patsy categorical/ordinal formulas don't match those of PTF_formula's formulas.
Iotas and gammas would be easy to visualize as a list of endpoints. Alphas would be less explicit this way, because they would become the first endpoint in a bucket, with the other endpoint being implied. Still, it's probably worth moving to lists so users don't have to type a bunch of redundant tuples.
oh duh, I can just do that |
would it break anything if you just add the greeks to the formula if provided?
I think about the theoretical progression as the ELRF residual being beginner, PTF residual being intermediate, and the BZ08 PTF residual being advanced. So do we really want something as advanced as BZ08 PTF in the main body in the development user guide? Would it be more suitable as an example? |
Kinda yeah. The formulation in my updated docs matches the output produced by fitting models with PTF_formula, with a minor difference from the authors' formulation. Using Patsy formulas to fit categorical variables directly leads to a fairly different formulation.
I think mixing the two would lead to coefficients that are harder to interpret. Side note, I should add something to make it clear that the formulation is different for the pathological example.
Maybe, but I don't have another example that uses PTF_formula. I think the functionality provided by PTF_formula is core to the authors' intentions; without it, we're left with categorical/ordinal only models. We'll have to come up with something to take its place. |
…ket. Gammas/iotas are the endpoints of each linear piece. Updated test_barnzehn accordingly
|
I changed PTF_formula to take period lists, not tuple lists. Docs are updating rn. |
would it really be that much different from what's written into class patsy():
def init(formula, alpha, gamma, iota)
self.formula = formula
if alpha:
self.formula += f'I(shenanigans{alpha}'
...standardizing gamma as in [1,2,3,6,8,11] instead of [12,24,36,72,96,132]. this allows you to get rid of having to pass in the triangle. |
Documentation Editing Concerns
I'd like to start here, just in case I have to close this PR to rework everything. This all took some time, in part because of my concerns over how to edit the docs. I found that merely opening a notebook in jupyter lab (invoked by
uv run --with jupyter jupyter lab) immediately changed many parts of the file. While I can still build the docs locally, I'm worried that this will break things upstream. Editing the notebook using a text editor is cumbersome outside of small edits, and would have made adding cells incredibly annoying. I couldn't find a safe way to edit things otherwise, so I went ahead and used jupyter lab. Please tell me if there's a better way of editing the docs, or if I am worrying about nothing.EDIT: I built the docs here. This seems like a non-issue.
Changes
This PR is intended to put any BarnettZehnwirth related issues or concerns in the ground. The documentation related to this estimator in user_guide/development has been changed in the following ways:
In addition, I changed how gamma and iota are passed to PTF_formula, and changed test_barnzehn accordingly.
To be changed
Before merging I would like to address some things, including implementation considerations raised earlier.
If any discussion runs long or major changes are suggested, we could also create a new branch and iterate from there.