Skip to content

Fix invalid typestring size #2797

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

Merged
merged 6 commits into from
Dec 21, 2012

Conversation

jayvius
Copy link
Contributor

@jayvius jayvius commented Dec 7, 2012

Revert to pre numpy 1.7 behavior where invalid typestring size was ignored and display future deprecate warning. This warning should eventually be changed to an error in future numpy versions.

Fix for #294

@njsmith
Copy link
Member

njsmith commented Dec 7, 2012

Awesome, thanks for working on this.

I believe this should be a regular DEPRECATE, not DEPRECATE_FUTUREWARNING (since the new behaviour in the future will be an exception, not silently altered results).

Needs a test.

@njsmith
Copy link
Member

njsmith commented Dec 15, 2012

Your tests are failing.

if (temp != NULL) {
if (temp->elsize != itemsize) {
if (DEPRECATE(msg) < 0) {
return -1;
Copy link
Member

Choose a reason for hiding this comment

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

missing a Py_DECREF(temp) 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.

oops...good catch :)

@certik
Copy link
Contributor

certik commented Dec 15, 2012

There are test failures here.

@certik
Copy link
Contributor

certik commented Dec 16, 2012

@jayvius, would you have time to finish this? This issue is a blocker for the 1.7 release.

@jayvius
Copy link
Contributor Author

jayvius commented Dec 16, 2012

Yes, I'm working on addressing all the issues with this pull request by
tomorrow.

On Sun, Dec 16, 2012 at 2:18 PM, Ondřej Čertík notifications@github.comwrote:

@jayvius https://github.com/jayvius, would you have time to finish
this? This issue is a blocker for the 1.7 release.


Reply to this email directly or view it on GitHubhttps://github.com//pull/2797#issuecomment-11422250.

@certik
Copy link
Contributor

certik commented Dec 17, 2012

Yes, I'm working on addressing all the issues with this pull request by
tomorrow.

Thanks! I really appreciate your help on this.

@charris
Copy link
Member

charris commented Dec 17, 2012

After this goes into 1.7 you might want to consider moving the deprecation tests into the new test_deprecation.py module.

@njsmith
Copy link
Member

njsmith commented Dec 17, 2012

I know there are other deprecation tests scattered about too, e.g. for the
diagonal changes - we should probably as a separate change see about
grepping for DeprecationWarning to consolidate them. (Having them in one
place seems useful for reviewing before the next release. Esp. if annotated
with comments about when they were introduced.)
On 17 Dec 2012 17:54, "Charles Harris" notifications@github.com wrote:

After this goes into 1.7 you might want to consider moving the deprecation
tests into the new test_deprecation.py module.


Reply to this email directly or view it on GitHubhttps://github.com//pull/2797#issuecomment-11452097.

@jayvius
Copy link
Contributor Author

jayvius commented Dec 17, 2012

Anyone know why 'g12' type would throw a warning on my Macbook but fail to throw a warning on the Travis build server? It seems like 12 shouldn't be a valid size for 'g' regardless of platform.

@rgommers
Copy link
Member

Don't know why the warning doesn't show up on Python <2.7, but on 2.7 and 3.x it looks like the test will fail anyway unless you run it with python -Wd.

@jayvius
Copy link
Contributor Author

jayvius commented Dec 17, 2012

It seems to make it through the tests just fine though on 2.7 and 3.x until
it hits the 'g12' test.

On Mon, Dec 17, 2012 at 1:59 PM, Ralf Gommers notifications@github.comwrote:

Don't know why the warning doesn't show up on Python <2.7, but on 2.7 and
3.x it looks like the test will fail anyway unless you run it with python
-Wd.


Reply to this email directly or view it on GitHubhttps://github.com//pull/2797#issuecomment-11458582.

@rgommers
Copy link
Member

Ah yes, because in NoseTester there's warnings.filterwarnings('always', category=DeprecationWarning).

Note though that this only works in the maintenance branches, in master it's warnings.filterwarnings('error', category=DeprecationWarning). So if you want to put this in master, you'll need to play with the warning settings.

@certik
Copy link
Contributor

certik commented Dec 21, 2012

Thanks Jay for fixing it. Tests still fail, so we need to figure out how to tests this, so that tests don't fail, before we can merge this.

@jayvius
Copy link
Contributor Author

jayvius commented Dec 21, 2012

Yeah, sorry about letting this one linger. I still can't figure out why the
test works on my Macbook and fails on the build server. Is it considered
bad practice to commit some debug logging and trigger a new Travis build
with the debug logging?

On Thu, Dec 20, 2012 at 6:36 PM, Ondřej Čertík notifications@github.comwrote:

Thanks Jay for fixing it. Tests still fail, so we need to figure out how
to tests this, so that tests don't fail, before we can merge this.


Reply to this email directly or view it on GitHubhttps://github.com//pull/2797#issuecomment-11598604.

@njsmith
Copy link
Member

njsmith commented Dec 21, 2012

Naw, go for it. Just remember to get rid of the debug logging again before
we merge :-)

On Fri, Dec 21, 2012 at 3:33 PM, Jay Bourque notifications@github.comwrote:

Yeah, sorry about letting this one linger. I still can't figure out why
the
test works on my Macbook and fails on the build server. Is it considered
bad practice to commit some debug logging and trigger a new Travis build
with the debug logging?

On Thu, Dec 20, 2012 at 6:36 PM, Ondřej Čertík notifications@github.comwrote:

Thanks Jay for fixing it. Tests still fail, so we need to figure out how
to tests this, so that tests don't fail, before we can merge this.


Reply to this email directly or view it on GitHub<
https://github.com/numpy/numpy/pull/2797#issuecomment-11598604>.


Reply to this email directly or view it on GitHubhttps://github.com//pull/2797#issuecomment-11617000.

Revert to pre numpy 1.7 behavior where invalid typestring size was ignored and isplay deprecate warning. This warning should eventually be changed to an error in future numpy versions.
- Refactor unit tests for invalid dtypes so that each test is on a separate line. This will make it easier to figure out which typestring is failing the unit test.
- Add Py_DECREF for temp variable
Add check to see if 12 bytes is a valid size for a long double
@jayvius
Copy link
Contributor Author

jayvius commented Dec 21, 2012

Weird...apparently a long double on Travis is 12 bytes. I didn't think that size was all that common.

That issue should be fixed, but now the python 3.1 build is failing on Travis due to an issue with nose. Any ideas?

@njsmith
Copy link
Member

njsmith commented Dec 21, 2012

Travis just dropped support for 3.1... and it looks like instead of, say, for example, not running builds on unsupported python versions, they're just running the build directly against their system py2.7. Which fails because our build script assumes a virtualenv, not a system python.

In short you can ignore this.

njsmith added a commit that referenced this pull request Dec 21, 2012
@njsmith njsmith merged commit 1e8fcdf into numpy:maintenance/1.7.x Dec 21, 2012
@njsmith
Copy link
Member

njsmith commented Dec 21, 2012

@certik - backport away.

@certik certik mentioned this pull request Dec 26, 2012
@certik
Copy link
Contributor

certik commented Dec 26, 2012

@njsmith, this PR was made against 1.7, so it is already backported. This time, we need to forwardport it: #2853.

@certik certik deleted the typestring_fix branch December 26, 2012 17:21
certik added a commit that referenced this pull request Dec 26, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants