Skip to content

TST: adapt table representation tests to numpy 2 - #16433

Merged
mhvk merged 1 commit into
astropy:mainfrom
neutrinoceros:table/compat/16423/numpy2_scalar_repr
May 24, 2024
Merged

TST: adapt table representation tests to numpy 2#16433
mhvk merged 1 commit into
astropy:mainfrom
neutrinoceros:table/compat/16423/numpy2_scalar_repr

Conversation

@neutrinoceros

@neutrinoceros neutrinoceros commented May 10, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor

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 Bot commented May 10, 2024 •
edited by taldcroft
Loading

Copy link
Copy Markdown
Contributor

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 Bot commented May 10, 2024

Copy link
Copy Markdown
Contributor

👋 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
Comment thread astropy/table/tests/test_mixin.py


def test_representation_representation():
@pytest.mark.parametrize(

@neutrinoceros neutrinoceros May 10, 2024

Copy link
Copy Markdown
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 🤷🏻‍♂️

@pllim pllim May 10, 2024

Copy link
Copy Markdown
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. 🤔

@neutrinoceros neutrinoceros May 10, 2024

Copy link
Copy Markdown
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.

@pllim pllim May 10, 2024

Copy link
Copy Markdown
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.

@taldcroft taldcroft May 11, 2024

Copy link
Copy Markdown
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 approved these changes May 11, 2024
View reviewed changes

@taldcroft taldcroft left a comment

Copy link
Copy Markdown
Member

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

mhvk commented May 11, 2024

Copy link
Copy Markdown
Contributor

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

neutrinoceros commented May 12, 2024

Copy link
Copy Markdown
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

neutrinoceros commented May 14, 2024

Copy link
Copy Markdown
Contributor Author

The io.misc label should be removed.

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

pllim commented May 15, 2024

Copy link
Copy Markdown
Member

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

@pllim

pllim commented May 23, 2024

Copy link
Copy Markdown
Member

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

mhvk
mhvk approved these changes May 24, 2024
View reviewed changes

@mhvk mhvk left a comment

Copy link
Copy Markdown
Contributor

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
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)
@neutrinoceros neutrinoceros mentioned this pull request Oct 15, 2024
Merged
1 task
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

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.

4 participants

@neutrinoceros @mhvk @pllim @taldcroft