Skip to content
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

SparseSeries.combine_first(Series) fails when combining with a Series #687

Closed
creeson opened this issue Jan 25, 2012 · 1 comment

Comments

@creeson
Copy link

commented Jan 25, 2012

Contrary to the documentation, the SparseSeries code, when doing a combine_first, actually assumes the other series is a SparseSeries. It performs a other.to_dense(), which fails on a Series.

def combine_first(self, other):
    """
    Combine Series values, choosing the calling Series's values
    first. Result index will be the union of the two indexes

    Parameters
    ----------
    other : Series

    Returns
    -------
    y : Series
    """
    dense_combined = self.to_dense().combine_first(other.to_dense())
    return dense_combined.to_sparse(fill_value=self.fill_value)

I changed my code locally to the following, which seems to work (though I'm sure there is a more elegant way):

def combine_first(self, other):
    """
    Combine Series values, choosing the calling Series's values
    first. Result index will be the union of the two indexes

    Parameters
    ----------
    other : Series

    Returns
    -------
    y : Series
    """
    if isinstance(other, SparseSeries):
        dense_combined = self.to_dense().combine_first(other.to_dense())
    elif isinstance(other, Series):
        dense_combined = self.to_dense().combine_first(other)
    return dense_combined.to_sparse(fill_value=self.fill_value)
wesm added a commit that referenced this issue Jan 25, 2012
BUG: fix SparseSeries.combine_first work when passed dense Series, ma…
…ke SparseDataFrame.combine_first work also, GH #687
@wesm

This comment has been minimized.

Copy link
Member

commented Jan 25, 2012

Good point. I also noticed that SparseDataFrame.combine_first is broken and had no unit test using it so I fixed that also in the above commit. The sparse data structures could use a lot more testing in general to make sure the API is reasonably compliant

@wesm wesm closed this Jan 25, 2012

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.