Open
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds comprehensive snapshot tests for etable and plotting functionality (coefplot/iplot) via syrupy, and migrates internal table generation from custom code to the maketables package.
Key Changes
- Migrated table functionality to use maketables package internally, replacing custom implementations
- Added snapshot tests for etable output across multiple formats (gt, tex, md, df, html)
- Added snapshot tests for coefplot/iplot across matplotlib and lets_plot backends
- Deprecated pf.dtable() and removed pf.make_table() in favor of maketables equivalents
Reviewed changes
Copilot reviewed 20 out of 24 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_plots_snapshot.py | New comprehensive snapshot tests for coefplot/iplot visual output |
| tests/test_etable_snapshot.py | New comprehensive snapshot tests for etable output formats |
| tests/test_plots.py | Removed (replaced by snapshot tests) |
| tests/test_summarise.py | Updated etable test calls to use new file_name parameter |
| tests/test_dtable.py | Updated expected values for maketables formatting and added deprecation warning test |
| tests/texfiles/test.tex | Updated expected LaTeX output reflecting new maketables formatting |
| pyfixest/report/summarize.py | Refactored to delegate to maketables for table generation |
| pyfixest/report/make_table.py | Removed (functionality moved to maketables) |
| pyfixest/report/init.py | Removed make_table export |
| pyfixest/estimation/decomposition.py | Updated to use maketables.MTable |
| pyfixest/estimation/feols_.py | Added type ignore comment for pandas typing |
| pyfixest/did/lpdid.py | Added type ignore comments for pandas typing |
| pyproject.toml | Replaced great_tables with maketables dependency, added snapshot marker |
| pixi.toml | Added snapshot environment with syrupy and lets-plot dependencies |
| pytest.ini | Added snapshot marker definition |
| docs/table-layout.qmd | Updated documentation for maketables migration |
| docs/quarto_example/QuartoExample.qmd | Added tabularx package requirement |
| docs/changelog.qmd | Documented migration to maketables with guide |
| .github/workflows/snapshot-tests.yaml | New CI workflow for snapshot testing |
Comments suppressed due to low confidence (1)
pyfixest/report/summarize.py:20
- The docstring mentions a removed parameter 'show_se_type' in the function signature at line 105, but this parameter has been removed from the actual function signature. The docstring should be updated to remove references to this parameter.
def etable(
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
tests/test_etable_snapshot.py
Outdated
| result = gelbach_decomposition.etable(type="df", digits=3) | ||
| assert result.to_string() == snapshot | ||
|
|
||
|
|
There was a problem hiding this comment.
Remove trailing whitespace from line 507.
Suggested change
| result = pf.dtable(sample_df, vars=["var1"], type="df") | ||
|
|
||
| # Creating the expected DataFrame | ||
| # Note: maketables formats count as "5.00" instead of "5" |
There was a problem hiding this comment.
This explanatory comment appears before a change that updates expected values. Consider moving this comment closer to the actual change or removing it if it's now clear from context.
Suggested change
| # Note: maketables formats count as "5.00" instead of "5" |
|
|
||
| ::: {.callout-warning} | ||
| ## Deprecation Notice | ||
| `pf.dtable()` will be deprecated in the future. Please use from the `maketables` package. |
There was a problem hiding this comment.
Incomplete sentence - missing the function name after 'Please use'. Should be 'Please use DTable() from the maketables package.'
Suggested change
| `pf.dtable()` will be deprecated in the future. Please use from the `maketables` package. | |
| `pf.dtable()` will be deprecated in the future. Please use `DTable()` from the `maketables` package. |
| snapshot-test: | ||
| name: "Snapshot Tests" | ||
| runs-on: macos-14 # ARM-based macOS | ||
| continue-on-error: true # Warn only - does not block merge |
There was a problem hiding this comment.
Snapshot tests that don't block merges may allow visual regressions to slip through. Consider making these blocking once stabilized.
Suggested change
| continue-on-error: true # Warn only - does not block merge | |
| continue-on-error: ${{ github.event_name == 'schedule' }} # Allow failures on scheduled runs; block merges on push/PR |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Add snapshot tests for etable and coefplot / iplot via syrupy.