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.

Sign up for GitHub

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

Jump to bottom

TST: adapt table representation tests to numpy 2 #16433

Merged
merged 1 commit into from
May 24, 2024
Merged

TST: adapt table representation tests to numpy 2 #16433

merged 1 commit into from
May 24, 2024

Conversation

neutrinoceros
Copy link
Contributor

@neutrinoceros neutrinoceros commented May 10, 2024 •
edited

Description

This pull request is to address part of #16423 (specifically table)
The first commit is shared with #16426 and #16427, so they can be merged in any order.

  • By checking this box, the PR author has requested that maintainers do NOT use the "Squash and Merge" button. Maintainers should respect this when possible; however, the final decision is at the discretion of the maintainer that merges the PR.

@neutrinoceros neutrinoceros mentioned this pull request May 10, 2024
Closed
@github-actions GitHub Actions
Copy link

github-actions bot commented May 10, 2024 •
edited by taldcroft

Thank you for your contribution to Astropy! 🌌 This checklist is meant to remind the package maintainers who will review this pull request of some common things to look for.

  • Do the proposed changes actually accomplish desired goals?
  • Do the proposed changes follow the Astropy coding guidelines?
  • Are tests added/updated as required? If so, do they follow the Astropy testing guidelines?
  • Are docs added/updated as required? If so, do they follow the Astropy documentation guidelines?
  • Is rebase and/or squash necessary? If so, please provide the author with appropriate instructions. Also see instructions for rebase and squash.
  • Did the CI pass? If no, are the failures related? If you need to run daily and weekly cron jobs as part of the PR, please apply the "Extra CI" label. Codestyle issues can be fixed by the bot.
  • Is a change log needed? If yes, did the change log check pass? If no, add the "no-changelog-entry-needed" label. If this is a manual backport, use the "skip-changelog-checks" label unless special changelog handling is necessary.
  • Is this a big PR that makes a "What's new?" entry worthwhile and if so, is (1) a "what's new" entry included in this PR and (2) the "whatsnew-needed" label applied?
  • At the time of adding the milestone, if the milestone set requires a backport to release branch(es), apply the appropriate "backport-X.Y.x" label(s) before merge.

@github-actions GitHub Actions
Copy link

👋 Thank you for your draft pull request! Do you know that you can use [ci skip] or [skip ci] in your commit messages to skip running continuous integration tests until you are ready?

@neutrinoceros neutrinoceros force-pushed the table/compat/16423/numpy2_scalar_repr branch from c866449 to 5b5b962 Compare May 10, 2024 07:01
neutrinoceros
neutrinoceros commented May 10, 2024
View reviewed changes
astropy/table/tests/test_mixin.py
@@ -756,56 +757,104 @@ def test_quantity_representation():
]


def test_representation_representation():
@pytest.mark.parametrize(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

note to reviewers: the diff is not minimal here because I started by refactoring as a parametrized test before I knew exactly how I was going to avoid duplication. In the end it's not necessary but I figure it doesn't hurt either, so I didn't undo it 🤷🏻‍♂️

Copy link
Member

Choose a reason for hiding this comment

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

The refactored version is harder to read by human though. 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, happy to revert the non-essential part if requested.

Copy link
Member

Choose a reason for hiding this comment

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

Let's see what @taldcroft says.

Copy link
Member

Choose a reason for hiding this comment

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

The refactor is OK. It is slightly less readable but I think that is outweighed by the benefit of having the tests separated so if one fails the rest still run.

@neutrinoceros neutrinoceros marked this pull request as ready for review May 10, 2024 08:17
@taldcroft taldcroft added this to the v6.1.1 milestone May 11, 2024
@taldcroft taldcroft added the backport-v6.1.x on-merge: backport to v6.1.x label May 11, 2024
taldcroft
taldcroft approved these changes May 11, 2024
View reviewed changes
Copy link
Member

@taldcroft taldcroft left a comment

Choose a reason for hiding this comment

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

This looks good to me for Table. @pllim is this good to merge, right milestone and all?

@mhvk
Copy link
Contributor

mhvk commented May 11, 2024

This is sad: since #15065 already fixed almost everthing to do with the format change, the changes here are in fact in there: see commit a2ae3fe. This even includes the parametrization. I suggest we merge #15065 before making further adjustments, to avoid these types of duplicated efforts.

@neutrinoceros
Copy link
Contributor Author

Damn, sorry I missed that. I looked up your PR a couple times for other changes and didn't find them there so I just stopped doing it just assumed it only dealt with docstring tests.
There's a clear tension in that we don't want to merge your whole PR until numpy 2.0.0 final comes out, but some real incompatibilities (tentatively described by #16423) should be addressed sooner if possible. This particular PR doesn't fall in that second category though, and could easily be dropped or reworked to avoid conflicts with yours.
I'll review #15065 now to avoid another mishap, but I still expect it won't get merged before 2.0.0, so please let me know what you'd rather we do with this one: merge as is, replace conflicting commits with cherry-picks from #15065 and then merge, or just close it ?

This was referenced May 13, 2024
Merged
Merged
@neutrinoceros neutrinoceros force-pushed the table/compat/16423/numpy2_scalar_repr branch from 5b5b962 to 3ffdf7e Compare May 14, 2024 06:23
@neutrinoceros
Copy link
Contributor Author

The io.misc label should be removed.

@pllim pllim added numpy-dev and removed io.misc labels May 14, 2024
@pllim
Copy link
Member

pllim commented May 15, 2024

@mhvk , how should we proceed? Please advise. Thanks!

@pllim
Copy link
Member

pllim commented May 23, 2024

I think this one is still waiting for response from @mhvk .

mhvk
mhvk approved these changes May 24, 2024
View reviewed changes
Copy link
Contributor

@mhvk mhvk left a comment

Choose a reason for hiding this comment

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

Let's just get this in - it is a duplication of my PR, but it's handier to do it in small steps.

@mhvk mhvk merged commit 144604e into astropy:main May 24, 2024
35 of 38 checks passed
meeseeksmachine pushed a commit to meeseeksmachine/astropy that referenced this pull request May 24, 2024
@mhvk @meeseeksmachine
Backport PR astropy#16433: TST: adapt table representation tests to n…
9d6ccfa 
…umpy 2
@meeseeksmachine meeseeksmachine mentioned this pull request May 24, 2024
Merged
@neutrinoceros neutrinoceros deleted the table/compat/16423/numpy2_scalar_repr branch May 24, 2024 05:56
pllim added a commit that referenced this pull request May 24, 2024
@pllim
Merge pull request #16494 from meeseeksmachine/auto-backport-of-pr-16…
f6f1c7e 
…433-on-v6.1.x

Backport PR #16433 on branch v6.1.x (TST: adapt table representation tests to numpy 2)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pllim pllim pllim left review comments

@taldcroft taldcroft taldcroft approved these changes

@mhvk mhvk mhvk approved these changes

@WilliamJamieson WilliamJamieson Awaiting requested review from WilliamJamieson

@matteobachetti matteobachetti Awaiting requested review from matteobachetti

Assignees
No one assigned
Labels
backport-v6.1.x on-merge: backport to v6.1.x no-changelog-entry-needed numpy-dev table testing
Projects
None yet
Milestone
v6.1.1
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@neutrinoceros @mhvk @pllim @taldcroft