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

ERR/API: Raise NotImplementedError when method is not implemented (issue: 7692) #11095

Closed
wants to merge 1 commit into from

Conversation

terrytangyuan
Copy link
Contributor

Fixed #7692

@terrytangyuan
Copy link
Contributor Author

@jreback Does this need a whatsnew note or test? Would the test to check whether this raises NotImplementedError?

@jreback
Copy link
Contributor

jreback commented Sep 14, 2015

tests

@terrytangyuan
Copy link
Contributor Author

@jreback thanks could you check the update again?

p = Panel(np.arange(3*4*5).reshape(3,4,5), items=['ItemA','ItemB','ItemC'], \
major_axis=pd.date_range('20130101',periods=4),minor_axis=list('ABCDE'))
d = p.sum(axis=1).ix[0]
with self.assertRaises(NotImplementedError):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's exercise this with all of the ops for now e.g .add/.sub etc

@terrytangyuan
Copy link
Contributor Author

Thanks. Done @jreback

@terrytangyuan
Copy link
Contributor Author

On green. Could you merge? @jreback

@jreback
Copy link
Contributor

jreback commented Sep 15, 2015

@terrytangyuan just a single ping is enough.

d = p.sum(axis=1).ix[0]
with self.assertRaises(NotImplementedError):
p.mul(d, axis=0)
with self.assertRaises(NotImplementedError):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

make a loop of all of the ops functions (hint, there are more)

@terrytangyuan
Copy link
Contributor Author

@jreback I apologize for the repetitive ping and thanks for the feedback. I've made the changes.

@@ -679,6 +679,8 @@ def _combine(self, other, func, axis=0):
return self._combine_frame(other, func, axis=axis)
elif np.isscalar(other):
return self._combine_const(other, func)
else:
raise NotImplementedError
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

give a meaningful message here

@jreback
Copy link
Contributor

jreback commented Sep 15, 2015

pls add a whatsnew note (Bug fixes) for 0.17.0

@jreback jreback added Error Reporting Incorrect or improved errors from pandas Reshaping Concat, Merge/Join, Stack/Unstack, Explode labels Sep 15, 2015
@terrytangyuan
Copy link
Contributor Author

@jreback Done. On green now. Thanks.

@@ -1100,7 +1100,7 @@ Bug Fixes
- Bug causes memory leak in time-series line and area plot (:issue:`9003`)

- Bug when setting a ``Panel`` sliced along the major or minor axes when the right-hand side is a ``DataFrame`` (:issue:`11014`)

- Bug that does not raise `NotImplementedError` when method is not implemented (:issue:`7692`)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not obvious what the change is

@terrytangyuan
Copy link
Contributor Author

Thanks for the review @jreback . Fixed and on green now.

@@ -679,6 +679,8 @@ def _combine(self, other, func, axis=0):
return self._combine_frame(other, func, axis=axis)
elif np.isscalar(other):
return self._combine_const(other, func)
else:
raise NotImplementedError('This method has not been implemented yet. ')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

needs an intelligeble message here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you mean by that?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

well, its already raising NotImplementedError, so would be nice to know why

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jreback How about [op-method-name] has not been implemented in Panel class yet. Please wait for the next release?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no, just say this type has not been implemented.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jreback any idea on how to access the op-method-name that's not implemented?

func returns <function na_op at 0x10358dde8> and func.__name__ returns na_op. Neither of these is something I expected, e.g. add, mul, etc. What is na_op BTW?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just say that this type(other) is not supported (eg. print that could, in this case it would be Series

@jreback jreback added this to the 0.17.0 milestone Sep 17, 2015
@@ -679,6 +679,8 @@ def _combine(self, other, func, axis=0):
return self._combine_frame(other, func, axis=axis)
elif np.isscalar(other):
return self._combine_const(other, func)
else:
raise NotImplementedError(str(type(other))+ ' is not supported. ')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

say in combine operations with type(self)

…t implemented (issue: 7692) (+1 squashed commit)

Squashed commits:
[9c56cd0] ERR/API: Raise NotImplementedError when Panel operator function is not implemented (issue: 7692) (+1 squashed commit)
Squashed commits:
[6c036e5] ERR/API: Raise NotImplementedError when Panel operator function is not implemented (issue: 7692) (+1 squashed commit)
Squashed commits:
[1e8ff32] ERR/API: Raise NotImplementedError when Panel operator function is not implemented (issue: 7692) (+1 squashed commit)
Squashed commits:
[7b487c7] ERR/API: Raise NotImplementedError when Panel operator function is not implemented (issue: 7692) (+1 squashed commit)
Squashed commits:
[9e7aa80] ERR/API: Raise NotImplementedError when method is not implemented (issue: 7692) (+1 squashed commit)
Squashed commits:
[17fadd1] ERR/API: Raise NotImplementedError when method is not implemented (issue: 7692) (+1 squashed commit)
Squashed commits:
[4b149df] ERR/API: Raise NotImplementedError when method is not implemented in Panel.py (issue: 7692) (+1 squashed commit)
Squashed commits:
[8832b74] ERR/API: Raise NotImplementedError when method is not implemented (issue: 7692) (+1 squashed commit)
Squashed commits:
[fc01bf8] ERR/API: Raise NotImplementedError when method is not implemented (issue: 7692) (+1 squashed commit)
Squashed commits:
[a04c4a2] raise error when not implemented
@terrytangyuan
Copy link
Contributor Author

@kawochen @jreback Thanks. Revised. On green now.

@jreback
Copy link
Contributor

jreback commented Sep 21, 2015

merged via 597aa42

thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Error Reporting Incorrect or improved errors from pandas Reshaping Concat, Merge/Join, Stack/Unstack, Explode
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants