REF: eliminate method _write() in json writers #36218
Conversation
pandas/io/json/_json.py
Outdated
| class SeriesWriter(Writer): | ||
| _default_orient = "index" | ||
| if not index and orient == "split": | ||
| self.obj = {"name": obj.name, "data": obj.values} |
There was a problem hiding this comment.
Hmm why do we need to do this now?
There was a problem hiding this comment.
I do not know. I simply used the original implementation and moved whatever assignments were in _write into __init__.
There was a problem hiding this comment.
Oh OK - I see what was done here. Let's not assign to self.obj as that will mess with the idempotency
There was a problem hiding this comment.
We assign to self.obj at the moment of the writer initialization, not when calling a method. So, I would argue that it is not harmful.
There was a problem hiding this comment.
I still think it is confusing to change what self.obj is for certain orients - would prefer not to do this
|
If there is a problem with updating |
|
Yea I don't think this is an improvement as it is better for the subclasses to override the implementation where appropriate rather than modifying the object that gets stored during initialization. Let's keep open for a few days as someone may disagree |
|
Would you consider another implementation? I created a new property |
pandas/io/json/_json.py
Outdated
| ) | ||
|
|
||
| @property | ||
| @abstractmethod | ||
| def obj_to_write(self): |
There was a problem hiding this comment.
Can you add an annotation for this?
| @@ -33,7 +35,7 @@ | |||
| # interface to/from | |||
| def to_json( | |||
| path_or_buf, | |||
| obj, | |||
| obj: NDFrame, | |||
There was a problem hiding this comment.
should this be FrameOrSeries?
There was a problem hiding this comment.
The problem with FrameOrSeries is that it does not allow changing the type of return value.
Inside the function to_json there is a statement, which changes object from series to frame, which is not allowed.
if orient == "table" and isinstance(obj, Series):
obj = obj.to_frame(name=obj.name or "values")
mypy error:
pandas/io/json/_json.py:78: error: Incompatible types in assignment (expression has type "DataFrame", variable has type "FrameOrSeries") [assignment]
I tried to use FrameOrSeriesUnion, which allows changing type from series to frame, but I get another mypy error.
Apparently for NDFrame only FrameOrSeries is a correct casting.
pandas/core/generic.py:2159: error: The erased type of self "Union[pandas.core.frame.DataFrame, pandas.core.series.Series]" is not a supertype of its class "pandas.core.generic.NDFrame" [misc]
So, I have no clue how to deal with typing here.
There was a problem hiding this comment.
@WillAyd i expected FrameOrSeriesUnion would work for this, am i missing something?
There was a problem hiding this comment.
@ivanovmg can you merge master and update the error message here? I don't think that line number is reflective of the latest code base
I think the error message is stemming from a mixture of using a Union and a TypeVar (the latter being somewhere in generic). Ideally should be cleaned up but I don't think needs to hold up this PR
There was a problem hiding this comment.
Here we go.
I merged master, then modified pandas/core/generic.py.
line 2164:
def to_json(
self: FrameOrSeriesUnion,
path_or_buf: Optional[FilePathOrBuffer] = None,
And replaced NDFrame with FrameOrSeriesUnion in _json.py.
The error would be:
pandas/core/generic.py:2164: error: The erased type of self "Union[pandas.core.frame.DataFrame, pandas.core.series.Series]" is not a supertype of its class "pandas.core.generic.NDFrame" [misc]
However, if I do not touch generic.py, then the following error with be thrown.
pandas/core/generic.py:2437: error: Argument "obj" to "to_json" has incompatible type "NDFrame"; expected "Union[DataFrame, Series]" [arg-type]
| @abstractmethod | ||
| def obj_to_write(self) -> Union[NDFrame, Dict[str, Any]]: | ||
| """Object to write in JSON format.""" | ||
|
|
There was a problem hiding this comment.
can you explicitly pass here
are the keys in the Dict necessarily str, or can they be any Index entries?
There was a problem hiding this comment.
Oh yes, actually the keys in the dictionary can be of IndexLabel type.
In FrameWriter the resulting dictionary keys can be tuples of strings in case of multicolumns.
In other cases the keys seem to be only strings.
|
nitpick about annotations, otherwise LGTM |
|
All checks are good after merging with master, except Windows: @jbrockmendel, unfortunately, I cannot fully resolve the issue with typing. Please see my comment above in relation to |
|
Thanks @ivanovmg |
black pandasgit diff upstream/master -u -- "*.py" | flake8 --diffAttempted to figure out what is wrong with the issue #36211.
I noticed that the private methods
_writeare defined in each child of classWriter, which simply re-defineobjand call the parent_writemethod.It turns out that the source of the recursion does not lie there, but I refactored the module slightly by removing the mediator
_writemethod. Instead I put the differences between the writers into their__init__method.It seems that source of the problem with the recursion mentioned in the bug report is in
pandas/_libs/src/ujson/python/objToJSON.c.