Conversation
PyRanges.__new__ was annotated as returning `pr.PyRanges | pd.DataFrame`, so every `pr.PyRanges(df)` call site was typed as a union and tripped type checkers on any downstream use expecting a strict PyRanges (e.g. passing the result to a `PyRanges`-typed parameter). The DataFrame fallback is only meaningful for pandas' internal frame reconstruction (`.drop()`, groupby aggregations, `.loc`, etc.), never for direct construction. Move it there: `__new__` now always returns a real PyRanges or raises ValueError with the missing columns, while `_constructor` gets a `_pyranges_constructor_with_fallback` classmethod that pandas calls internally and that still degrades to a DataFrame when a required column is missing — mirroring how geopandas splits GeoDataFrame.__init__ from its `_constructor_with_fallback`. RangeFrame.drop/drop_and_return/reindex switched from `self.__class__(...)` to `self._constructor(...)` so the degrade-to-DataFrame behavior (used by .drop() on PyRanges) still works now that __new__ raises instead of silently downgrading. A few call sites that drop non-required columns got `cast(...)` since they can never hit the fallback. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015mFnfFDemzwoEZDLJshzhx
pyright flagged the three self._constructor(...) calls in RangeFrame as "Object of type Series[Any] is not callable": pandas-stubs does not declare _constructor on DataFrame, so the attribute resolved through DataFrame's __getattr__ (column access) instead. It was not just a typing problem. pandas 3.0's DataFrame._constructor returns DataFrame rather than type(self), and RangeFrame does not override it, so RangeFrame.drop/drop_and_return/reindex (and combine_interval_columns through it) had started returning plain DataFrames instead of RangeFrames. Move the fallback to an explicit _constructor_with_fallback classmethod that RangeFrame defines (always builds cls) and PyRanges overrides (degrades to a DataFrame when a required column is missing, and is what PyRanges._constructor hands to pandas). The three methods call that hook, so RangeFrame keeps its type while PyRanges keeps the degrade-to-DataFrame behaviour. drop_and_return loses its TypeVar, which the "| pd.DataFrame" in the return type had already made unusable without a cast; every call site already casts. Also gitignore the files the doctests write into the repo root, which until now were left behind as untracked after every test run. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
7c85f69 to
c3ae28f
Compare
|
Did you fix it? I am a bit out of the loop here. Is it ready for submission? |
|
Not quite. I decided to benchmark before merging, and that opened a whole other can of worms. This PR takes away a little speed, but also, the way in which PyRanges are initiated (even pre-PR) is improvable. I'm running a few ideas. Only when I figured out that part, I will come back to this. Leave this here. If you want to roll back to your last commit, feel free. |
|
@endrebak for me, this is good to go now. @endrebak let me know if you want to review them, in which case I'll leave everything open. Note PR174 currently doesn't target master, must be changed to master after merging 171 |
2 participants
PyRanges.new was annotated as returning
pr.PyRanges | pd.DataFrame, so everypr.PyRanges(df)call site was typed as a union and tripped type checkers on any downstream use expecting a strict PyRanges (e.g. passing the result to aPyRanges-typed parameter).The DataFrame fallback is only meaningful for pandas' internal frame reconstruction (
.drop(), groupby aggregations,.loc, etc.), never for direct construction. Move it there:__new__now always returns a real PyRanges or raises ValueError with the missing columns, while_constructorgets a_pyranges_constructor_with_fallbackclassmethod that pandas calls internally and that still degrades to a DataFrame when a required column is missing — mirroring how geopandas splits GeoDataFrame.init from its_constructor_with_fallback.RangeFrame.drop/drop_and_return/reindex switched from
self.__class__(...)toself._constructor(...)so the degrade-to-DataFrame behavior (used by .drop() on PyRanges) still works now that new raises instead of silently downgrading. A few call sites that drop non-required columns gotcast(...)since they can never hit the fallback.@marco-mariotti Thanks for the Claude trial. It only needed a bit of nudging to find the optimal design and then it seems to have performed flawlessly. Will reread some other day when I am not so tired.