10000 Improve documentation of handing non-finite fitting data by WilliamJamieson · Pull Request #13257 · astropy/astropy · GitHub
[go: up one dir, main page]
Skip to content

Improve documentation of handing non-finite fitting data #13257

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
Merged
merged 5 commits into from
May 17, 2022
Merged

Improve documentation of handing non-finite fitting data #13257

merged 5 commits into from
May 17, 2022

Conversation

WilliamJamieson
Copy link
Contributor
@WilliamJamieson WilliamJamieson commented May 17, 2022 •
edited by pllim
Loading

Description

This PR addresses the changes decided upon for astropy during an offline discussion of astropy/photutils#1345, which amount to adding documentation how to resolve the error and an improved error message.

Checklist for package maintainer(s)

This checklist is meant to remind the package maintainer(s) who will review this pull request of some common things to look for. This list is not exhaustive.

  • 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 "When to rebase and squash commits".
  • 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.
  • 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 astropy-bot check might be missing; 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.

@WilliamJamieson WilliamJamieson added the 💤 backport-v5.1.x on-merge: backport to v5.1.x label May 17, 2022
@WilliamJamieson WilliamJamieson added this to the v5.1 milestone May 17, 2022
larrybradley
larrybradley approved these changes May 17, 2022
View reviewed changes
Copy link
Member
@larrybradley larrybradley left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

hamogu
hamogu requested changes May 17, 2022
View reviewed changes
Copy link
Member
@hamogu hamogu left a comment

Choose a reason for hiding this comment

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

I think that's good addition. Only flagging one work with might by a typo (but I'm not a native English speaker, if you confirm that's intentional, that's also OK).

@WilliamJamieson @hamogu
Fix typo
c7117ca 
Co-authored-by: Hans Moritz Günther <moritz.guenther@gmx.de>
perrygreenfield
perrygreenfield approved these changes May 17, 2022
View reviewed changes
Copy link
Member
@perrygreenfield perrygreenfield left a comment

Choose a reason for hiding this comment

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

LGTM

@pllim
Copy link
Member
pllim commented May 17, 2022

@hamogu , does this look good to you now?

pllim
pllim reviewed May 17, 2022
View reviewed changes
Copy link
Member
@pllim pllim left a comment

Choose a reason for hiding this comment

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

It is slightly less confusing to put "for 1D" or "for 2D" before the example, for those who speed-read through documentation.

@WilliamJamieson @pllim
Apply suggestions from code review
9c7b409 
Co-authored-by: P. L. Lim <2090236+pllim@users.noreply.github.com>
larrybradley
larrybradley reviewed May 17, 2022
View reviewed changes
@WilliamJamieson @larrybradley
fix another typo
347bdca 
Co-authored-by: Larry Bradley <larry.bradley@gmail.com>
@pllim pllim merged commit 269fc70 into astropy:main May 17, 2022
@pllim
Copy link
Member
pllim commented May 17, 2022

Thanks! 😌

meeseeksmachine pushed a commit to meeseeksmachine/astropy that referenced this pull request May 17, 2022
@pllim @meeseeksmachine
Backport PR astropy#13257: Improve documentation of handing non-finit…
9c43c18 
…e fitting data
@meeseeksmachine meeseeksmachine mentioned this pull request May 17, 2022
Merged
@pllim
Copy link
Member
pllim commented May 17, 2022

Yay, backport label appears to work again!

This was referenced May 17, 2022
Closed
Closed
pllim added a commit that referenced this pull request May 17, 2022
@pllim
Merge pull request #13258 from meeseeksmachine/auto-backport-of-pr-13…
d6a7ed9 
…257-on-v5.1.x

Backport PR #13257 on branch v5.1.x (Improve documentation of handing non-finite fitting data)
@WilliamJamieson WilliamJamieson deleted the resolve_fitting_nans branch May 17, 2022 19:49
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

@larrybradley larrybradley larrybradley approved these changes

@hamogu hamogu hamogu approved these changes

@perrygreenfield perrygreenfield perrygreenfield approved these changes

@nden nden Awaiting requested review from nden

Assignees
No one assigned
Labels
modeling no-changelog-entry-needed 💤 backport-v5.1.x on-merge: backport to v5.1.x
Projects
None yet
Milestone
v5.1
Development

Successfully merging this pull request may close these issues.
5 participants
@WilliamJamieson @pllim @hamogu @perrygreenfield @larrybradley
0