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

Remove unitstripswarnings #3

Merged
merged 16 commits into from
Aug 28, 2018
Merged

Conversation

andrewgsavage
Copy link
Collaborator

Lazy way to get tests going again

@andrewgsavage andrewgsavage changed the title Remove unitstrips from isna() Remove unitstripswarnings Aug 27, 2018
@andrewgsavage
Copy link
Collaborator Author

This should remove most Unitstripwarnings. There's none in the documentation at least! Any thoughts @znicholls ?

Unitstripwarnings notes

They're thrown whenever a method named _array* is called on a quantity. I've been either avoiding doing this (eg by going to quantity.magnitude) or using with warnings filtered when the warnings won't be an issue.

That being said there's still a few cases where the warning is raised without going through the PintArray, so might need to look at changing how/when the warning is raised.

@znicholls
Copy link
Owner

@andrewgsavage to get tests passing again see andrewgsavage#4

@znicholls
Copy link
Owner

znicholls commented Aug 28, 2018

Otherwise I'd rather not silence UnitStripWarnings in the tests, let's leave them if they're there

@@ -146,7 +147,35 @@ class TestInterface(base.BaseInterfaceTests):
pass

class TestMethods(base.BaseMethodsTests):
pass

@pytest.mark.filterwarnings("ignore::pint.UnitStrippedWarning")
Copy link
Owner

Choose a reason for hiding this comment

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

I'd like to keep these warnings popping up unless there's a good reason to silence them?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The ones I've been silencing should be occurring without going through the PintArray, so there's nothing we can do in pint_array.py to get rid of them - That'd have to be done in quantity.py.

Main reason I'd been silencing them was to get through them all easier. I think it's easier to spot whether a future change starts creating a UnitStrippedWarning like this.

I'm not fussed either way.

Copy link
Owner

Choose a reason for hiding this comment

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

ah ok makes sense, if they're not an issue then we leave it for now


self.assert_series_equal(result, expected)

@pytest.mark.filterwarnings("ignore::pint.UnitStrippedWarning")
Copy link
Owner

Choose a reason for hiding this comment

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

I'd like to keep these warnings popping up unless there's a good reason to silence them?

@@ -275,8 +304,25 @@ class TestReshaping(base.BaseReshapingTests):
pass

class TestSetitem(base.BaseSetitemTests):
pass
@pytest.mark.parametrize('setter', ['loc', None])
@pytest.mark.filterwarnings("ignore::pint.UnitStrippedWarning")
Copy link
Owner

Choose a reason for hiding this comment

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

I'd like to keep these warnings popping up unless there's a good reason to silence them?

@@ -334,10 +380,15 @@ def test_pintarray_creation(self):
for y in ys:
self.assertQuantityAlmostEqual(x,y.data)


@pytest.mark.filterwarnings("ignore::pint.UnitStrippedWarning")
Copy link
Owner

Choose a reason for hiding this comment

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

I'd like to keep these warnings popping up unless there's a good reason to silence them?

@andrewgsavage
Copy link
Collaborator Author

So is this OK to merge then?

@znicholls
Copy link
Owner

yep sorry

@znicholls znicholls merged commit 8e2d3cd into znicholls:master Aug 28, 2018
@andrewgsavage
Copy link
Collaborator Author

Does it's time to get this into pint then? Exciting!

@znicholls
Copy link
Owner

Does it's time to get this into pint then?

ya

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.

2 participants