Skip to content

Conversation

@damandhaliwal
Copy link
Member

Hi @s3alfisc,

Putting in a draft PR here for your eyes. Let me know if you have any feedback on the API setup.

Here's what's left (I'm on it):

  • Add documentation
  • Benchmarking
  • Visualizations
  • Clean outputs/reporting

Just wanted to see if you had any bigger API design level changes before I went down the rabbit hole and implemented everything

@damandhaliwal damandhaliwal marked this pull request as draft December 19, 2025 19:50
@codecov
Copy link

codecov bot commented Dec 19, 2025

Codecov Report

❌ Patch coverage is 51.23457% with 158 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
pyfixest/report/visualize.py 2.08% 141 Missing ⚠️
pyfixest/estimation/sensitivity.py 90.00% 17 Missing ⚠️
Flag Coverage Δ
core-tests 73.89% <51.23%> (?)
tests-extended ?
tests-vs-r 17.12% <8.64%> (-0.45%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
pyfixest/estimation/feglm_.py 79.27% <100.00%> (+52.71%) ⬆️
pyfixest/estimation/feiv_.py 86.91% <100.00%> (+66.16%) ⬆️
pyfixest/estimation/feols_.py 86.89% <100.00%> (+26.36%) ⬆️
pyfixest/estimation/fepois_.py 90.70% <100.00%> (+34.26%) ⬆️
pyfixest/estimation/sensitivity.py 90.00% <90.00%> (ø)
pyfixest/report/visualize.py 38.87% <2.08%> (-21.83%) ⬇️

... and 40 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@damandhaliwal
Copy link
Member Author

@s3alfisc the only thing I did not add in (didn't know if it was too useful/would add bloat) are the default sensitivity plots that PySensemakr makes. But I can add them if you would like

@s3alfisc
Copy link
Member

s3alfisc commented Jan 4, 2026

Could you add some defensive checks (plus tests in test_errors_warnigns) that throw an error when users try to use the sensitivity method for models other than the OLS class? Add a self._supports_sensitivity_analysis = True for Feols, but set it to False in the __init__ of the other model classes? Else all other model classes will inherit the sensitivity method from Feols and incorrectly run the analysis?

@s3alfisc
Copy link
Member

s3alfisc commented Jan 4, 2026

@s3alfisc the only thing I did not add in (didn't know if it was too useful/would add bloat) are the default sensitivity plots that PySensemakr makes. But I can add them if you would like

Probably makes sense to add them, but we can do it in a separate PR. Not sure if these are really widely used in practice.

@s3alfisc
Copy link
Member

s3alfisc commented Jan 4, 2026

We should add an example on how to use the method to the quickstart notebook.

Last - it would be great to write a "how to" notebook - what cam you do with the method, why is it needed, how does it work, maybe why the authors argue it is better than other methods (as e.g. Oster)? Maybe something like this, but even more to the point?

Note that this can also be done in a later PR.

s3alfisc and others added 8 commits January 4, 2026 12:14
# Please enter a commit message to explain why this merge is necessary,
# especially if it merges an updated upstream into a topic branch.
#
# Lines starting with '#' will be ignored, and an empty message aborts
# the commit.
- Add treatment parameter to SensitivityAnalysis.summary() method
- Fix unbound plot_estimate variable in ovb_contour_plot for t-value sensitivity
- Add 10 tests for contour and extreme plotting functions
@damandhaliwal damandhaliwal marked this pull request as ready for review January 16, 2026 00:08
@damandhaliwal
Copy link
Member Author

Last - it would be great to write a "how to" notebook - what cam you do with the method, why is it needed, how does it work, maybe why the authors argue it is better than other methods (as e.g. Oster)? Maybe something like this, but even more to the point?

I have added everything but this. I think I will add this in a separate PR. I can draft an issue for this once we finalize and merge this PR

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