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

CI/TST: fix failing tests in py37_np_dev #31091

Merged
merged 4 commits into from
Jan 17, 2020

Conversation

ShaharNaveh
Copy link
Member

No description provided.

@ShaharNaveh ShaharNaveh changed the title CI/TST: fix failing tests CI/TST: fix failing tests in py37_np_dev Jan 17, 2020
@WillAyd WillAyd added the CI Continuous Integration label Jan 17, 2020
Copy link
Member

@WillAyd WillAyd left a comment

Choose a reason for hiding this comment

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

lgtm if green

@WillAyd
Copy link
Member

WillAyd commented Jan 17, 2020

I think would need to back port as well

@ShaharNaveh
Copy link
Member Author

I think would need to back port as well

I think we do, as this error message is an external error message.

@ShaharNaveh
Copy link
Member Author

I think would need to back port as well

I think we do, as this error message is an external error message.

@jorisvandenbossche I am with you on not testing the error messages from external packages, here we got "lucky" (only 5 instances) but could easily be 40+ (and even more) if something else would have changed.

Any idea on how to mark error messages from external packages?

@ShaharNaveh
Copy link
Member Author

I think would need to back port as well

I think we do, as this error message is an external error message.

@jorisvandenbossche I am with you on not testing the error messages from external packages, here we got "lucky" (only 5 instances) but could easily be 40+ (and even more) if something else would have changed.

Any idea on how to mark error messages from external packages?

Posted in the wrong thread 🤦‍♂️

@ShaharNaveh
Copy link
Member Author

gentle ping @WillAyd ; Linux py37_np_dev is green :)

@@ -218,7 +218,7 @@ def test_take_fill_value():
with pytest.raises(ValueError, match=msg):
idx.take(np.array([1, 0, -5]), fill_value=True)

msg = "index -5 is out of bounds for size 4"
msg = "index -5 is out of bounds for( axis 0 with|) size 4"
Copy link
Member

Choose a reason for hiding this comment

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

nitpick: small preference for "( axis 0 with)?" over "( axis 0 with|)"

Copy link
Member

Choose a reason for hiding this comment

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

(ok for follow-up, since getting the CI working quickly will be nice)

@WillAyd WillAyd added this to the 1.0.0 milestone Jan 17, 2020
@WillAyd WillAyd merged commit 0f04c6a into pandas-dev:master Jan 17, 2020
@lumberbot-app
Copy link

lumberbot-app bot commented Jan 17, 2020

Owee, I'm MrMeeseeks, Look at me.

There seem to be a conflict, please backport manually. Here are approximate instructions:

  1. Checkout backport branch and update it.
$ git checkout 1.0.x
$ git pull
  1. Cherry pick the first parent branch of the this PR on top of the older branch:
$ git cherry-pick -m1 0f04c6af5eea56f69d046eb1603c25f4b8d45919
  1. You will likely have some merge/cherry-pick conflict here, fix them and commit:
$ git commit -am 'Backport PR #31091: CI/TST: fix failing tests in py37_np_dev'
  1. Push to a named branch :
git push YOURFORK 1.0.x:auto-backport-of-pr-31091-on-1.0.x
  1. Create a PR against branch 1.0.x, I would have named this PR:

"Backport PR #31091 on branch 1.0.x"

And apply the correct labels and milestones.

Congratulation you did some good work ! Hopefully your backport PR will be tested by the continuous integration and merged soon!

If these instruction are inaccurate, feel free to suggest an improvement.

@ShaharNaveh ShaharNaveh deleted the FIX-NUMPY-DEV-CI branch January 17, 2020 07:46
@WillAyd
Copy link
Member

WillAyd commented Jan 17, 2020

Looks like need to manually backport @Momisbestfriend mind doing that?

@ShaharNaveh ShaharNaveh mentioned this pull request Jan 17, 2020
5 tasks
ShaharNaveh pushed a commit to ShaharNaveh/pandas that referenced this pull request Jan 17, 2020
@ShaharNaveh
Copy link
Member Author

ShaharNaveh commented Jan 17, 2020

Looks like need to manually backport @MomIsBestFriend mind doing that?

@WillAyd sure!

@ShaharNaveh
Copy link
Member Author

ShaharNaveh commented Jan 17, 2020

I just checked it again, and this is not an external error message.

f"index {key} is out of bounds for axis 0 with size {len(self)}"

And it's located in:

# class RangeIndex(Int64Index):

def __getitem__(self, key):
        """
        Conserve RangeIndex type for scalar and slice keys.
        """
        if isinstance(key, slice):
            new_range = self._range[key]
            return self._simple_new(new_range, name=self.name)
        elif is_integer(key):
            new_key = int(key)
            try:
                return self._range[new_key]
            except IndexError:
                raise IndexError(
                    f"index {key} is out of bounds for axis 0 with size {len(self)}"
                )
        elif is_scalar(key):
            raise IndexError(
                "only integers, slices (`:`), "
                "ellipsis (`...`), numpy.newaxis (`None`) "
                "and integer or boolean "
                "arrays are valid indices"
            )
        # fall back to Int64Index
        return super().__getitem__(key)

I think that a future version of numpy will break pd.RangeIndex.

cc (@WillAyd, @jreback, @jbrockmendel)

@jorisvandenbossche
Copy link
Member

this is not an external error message.

That's only for RangeIndex (where we mimic numpy's behaviour and error messages), for all other cases it comes from numpy I think?

I think that a future version of numpy will break pd.RangeIndex.

How do you mean?

@ShaharNaveh
Copy link
Member Author

How do you mean?

@jorisvandenbossche Now that you explained that

That's only for RangeIndex (where we mimic numpy's behaviour and error messages), for all other cases it comes from numpy I think?

I don't think that a future version of numpy will break RangeIndex, I didn't understand what was going on before, but now I do.

Thank you for your explanation :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI Continuous Integration
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants