Skip to content

BUG: fix comparing NaN quantity with TimeDelta object - #15830

Merged
mhvk merged 2 commits into
astropy:mainfrom
neutrinoceros:time/bug/comp_nan_timedelta
Jan 10, 2024
Merged

BUG: fix comparing NaN quantity with TimeDelta object#15830
mhvk merged 2 commits into
astropy:mainfrom
neutrinoceros:time/bug/comp_nan_timedelta

Conversation

@neutrinoceros

@neutrinoceros neutrinoceros commented Jan 9, 2024

Copy link
Copy Markdown
Contributor

Description

Fixes #15230.
The patch is inspired from @mhvk's comment in #15230 (comment)

  • 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 changed the title TST: add a regression test for bug 15230 BUG: fix comparing NaN quantity with TimeDelta object Jan 9, 2024
@github-actions github-actions Bot added the time label Jan 9, 2024
@github-actions

github-actions Bot commented Jan 9, 2024 •
edited by mhvk
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?
  • Is a milestone set? Milestone must be set but we cannot check for it on Actions; do not let the green checkmark fool you.
  • 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 Jan 9, 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 time/bug/comp_nan_timedelta branch from c0b1e4f to ae7e164 Compare January 9, 2024 10:03
taldcroft
taldcroft reviewed Jan 9, 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 basically good to me. Any comments @mhvk?

Comment thread astropy/time/formats.py Resolved
Comment thread astropy/time/tests/test_delta.py
Comment on lines +117 to +125
assert not dt1 < dt2
assert not dt1 <= dt2
assert not dt1 > dt2

@taldcroft taldcroft Jan 9, 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.

What happens if the order is reversed? Everything still working?

@neutrinoceros neutrinoceros Jan 9, 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.

You mean inverting left and right operands ? I didn't think about it, I'll update the tests to cover all cases

@neutrinoceros neutrinoceros Jan 9, 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.

done

taldcroft
taldcroft reviewed Jan 9, 2024
View reviewed changes
Comment thread docs/changes/time/15830.bugfix.rst Resolved
Comment thread astropy/time/formats.py Resolved
@neutrinoceros
neutrinoceros force-pushed the time/bug/comp_nan_timedelta branch from 776b8eb to 1123af8 Compare January 9, 2024 12:49
@neutrinoceros
neutrinoceros marked this pull request as ready for review January 9, 2024 12:50
@mhvk mhvk added the Bug label Jan 9, 2024
mhvk
mhvk approved these changes Jan 9, 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.

Looks good, thanks! I started wondering a bit about whether np.isnan should work, and whether one should explicitly test initializing with np.nan, but ended up concluding that what you have is just right: we fix the reported bug.

A question still whether one should backport this - my tendency would be not to, since it is a (small) API change as well. But happy to be convinced otherwise.

@mhvk mhvk added this to the v6.1.0 milestone Jan 9, 2024
@taldcroft

taldcroft commented Jan 10, 2024

Copy link
Copy Markdown
Member

@mhvk - I'd say no backport and just go for 6.1 due to the small API change you mentioned. This one is not worth fighting for a backport.

@taldcroft

taldcroft commented Jan 10, 2024

Copy link
Copy Markdown
Member

And the updates look good to me as well, thanks @neutrinoceros !

@taldcroft

taldcroft commented Jan 10, 2024

Copy link
Copy Markdown
Member

I see that RTD CI is still Pending. @pllim what can we do about that?

@pllim

pllim commented Jan 10, 2024

Copy link
Copy Markdown
Member

I restarted RTD build. It got a weird error, hopefully transient. 🤞

@mhvk

mhvk commented Jan 10, 2024

Copy link
Copy Markdown
Contributor

Looks all green now, so let's get this in!

@mhvk
mhvk merged commit f9a61f1 into astropy:main Jan 10, 2024
@neutrinoceros
neutrinoceros deleted the time/bug/comp_nan_timedelta branch January 10, 2024 20:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@taldcroft taldcroft taldcroft left review comments
@mhvk mhvk mhvk approved these changes

Assignees

No one assigned

Labels

Bug time

Projects

None yet

Milestone

v6.1.0

Development

Successfully merging this pull request may close these issues.

TypeError when comparing TimeDelta and a nan Quantity

4 participants

@neutrinoceros @taldcroft @pllim @mhvk