New modeling tests and minor bug fixes by WilliamJamieson · Pull Request #12232 · astropy/astropy

WilliamJamieson · 2021-10-05T13:24:35Z

Description

Blocked by:

Fix for doctest failure #12235

This pull request adds and extends tests so that some parts of modeling which were not covered by unit tests are now covered by unit tests. Additionally, during this process several minor bugs were discovered and corrected; moreover, in a few places I discovered and removed code which would be impossible to actually execute. All of the changes to the unit testing serve to increase the test coverage of modeling with a focus on ensuring that all of the provided models are better covered by unit tests.

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 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 · 2021-10-05T17:16:50Z

Tagging @nden and @perrygreenfield for review.

astropy/modeling/convolution.py

astropy/modeling/functional_models.py

astropy/modeling/parameters.py

astropy/modeling/polynomial.py

astropy/modeling/tests/test_functional_models.py

nden · 2021-10-07T14:15:42Z

I think once the prior and posterior are added to copy and the commits are squashed this is ready for merging.

perrygreenfield

Generally minor comments.

astropy/modeling/convolution.py

astropy/modeling/tests/test_functional_models.py

astropy/modeling/tests/test_rotations.py

perrygreenfield

LGTM

WilliamJamieson · 2021-10-08T21:56:59Z

This has been rebased on #12235 in order to fix the failing doc test. So #12235 should be merged before this test is.

nstarman · 2021-10-09T19:22:14Z

docs/changes/modeling/12232.bugfix.rst

@@ -0,0 +1 @@
+Minor bugfixes and improvements to modeling.


Bug-fixes generally merit more specifics.

Updated the changelog to be more specific. See the conversation below for more details.

pllim · 2021-10-11T15:07:19Z

docs/changes/modeling/12232.bugfix.rst

@@ -0,0 +1,21 @@
+Minor bugfixes and improvements to modeling including the following:


Each bullet entry here should be a new fragment file, right, @saimn or @bsipocz ?

Also, only bug fix should be under bugfix category. But that said... usually things like typo fix and test clean-up do not need a change log. Refactoring only needs change log if it affects user-facing API.

p.s. In the future, if possible, please refrain from opening a giant PR with 2000 lines changed. Given the different entries in this change log, I assume some of them could have been independent smaller PRs?

All of these changes were just the natural consequences of my goal to increase the general code coverage of modeling. The vast majority of the lines changed are just more unit tests for modeling. This PR is the result of my "spare moments" review/maintenance of modeling of the past couple of months. If anything was particularly serious I would have submitted a PR with the fixes when I found it.

The changes in the PR to the modeling code are fixes to minor bugs uncovered when extending code coverage, or the discovery of reachable code while trying to extend code coverage. I suspect that the majority of the bugs were in code that no one is currently using as they would have created obvious errors if someone actually exercised them.

Updates to better test functional_models Increased coverage of mappings added more coverage to parameters Fixed broken test Increased parameters test coverage Added tests to fully cover physical_models Added tests to powerlaws. Added tests to cover rotations Added more tabular tests Added more coverage in a bunch of small places Added tests and fixes for projections Added tests for polynomial Added optimizer tests Fixed broken test under python 3.8 Fixed testing of abstract methods Added changelog Made use of `_validate_domain_window` in polynomial more consitent and fixed domain/window bug. Removed print from tests Fixed prior/posterior bug in Parameter __init__ and copy

The requested changes were implemented.

github-actions bot added the modeling label Oct 5, 2021

WilliamJamieson force-pushed the feature/increase_modeling_test_coverage branch from 2631b18 to a854f9c Compare October 5, 2021 13:55

WilliamJamieson force-pushed the feature/increase_modeling_test_coverage branch from a854f9c to dfd74be Compare October 5, 2021 17:36

nstarman requested review from nden and perrygreenfield October 6, 2021 03:30

nden reviewed Oct 7, 2021

View reviewed changes

WilliamJamieson force-pushed the feature/increase_modeling_test_coverage branch 3 times, most recently from 7543b4b to d2aea8a Compare October 7, 2021 18:20

perrygreenfield reviewed Oct 7, 2021

View reviewed changes

perrygreenfield approved these changes Oct 8, 2021

View reviewed changes

WilliamJamieson force-pushed the feature/increase_modeling_test_coverage branch from 08fc933 to 6591eb8 Compare October 8, 2021 21:23

nstarman modified the milestone: v5.0 Oct 9, 2021

nstarman reviewed Oct 9, 2021

View reviewed changes

WilliamJamieson force-pushed the feature/increase_modeling_test_coverage branch from 6591eb8 to 27ce579 Compare October 11, 2021 14:38

pllim added this to the v5.0 milestone Oct 11, 2021

pllim previously requested changes Oct 11, 2021

View reviewed changes

nstarman mentioned this pull request Oct 11, 2021

Fix for doctest failure #12235

Merged

9 tasks

WilliamJamieson force-pushed the feature/increase_modeling_test_coverage branch from 27ce579 to 34f1258 Compare October 12, 2021 22:51

WilliamJamieson force-pushed the feature/increase_modeling_test_coverage branch 5 times, most recently from b1624e3 to 31a1702 Compare October 28, 2021 20:57

WilliamJamieson added 2 commits October 29, 2021 21:55

Addressing perrygreenfield's review comments.

b407566

WilliamJamieson force-pushed the feature/increase_modeling_test_coverage branch from 31a1702 to b407566 Compare October 30, 2021 01:56

nden approved these changes Oct 30, 2021

View reviewed changes

nden merged commit d2bc237 into astropy:main Oct 30, 2021

WilliamJamieson deleted the feature/increase_modeling_test_coverage branch November 1, 2021 13:31

neutrinoceros mentioned this pull request Jul 2, 2024

TST: fix a test for compatibility with Python 3.13 #16659

Merged

1 task

		@@ -0,0 +1,21 @@
		Minor bugfixes and improvements to modeling including the following:

Uh oh!

Conversation

WilliamJamieson commented Oct 5, 2021 • edited by pllim Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Checklist for package maintainer(s)

Uh oh!

WilliamJamieson commented Oct 5, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nden commented Oct 7, 2021

Uh oh!

perrygreenfield left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

perrygreenfield left a comment

Choose a reason for hiding this comment

Uh oh!

WilliamJamieson commented Oct 8, 2021

Uh oh!

nstarman Oct 9, 2021

Choose a reason for hiding this comment

Uh oh!

WilliamJamieson Oct 11, 2021

Choose a reason for hiding this comment

Uh oh!

pllim Oct 11, 2021

Choose a reason for hiding this comment

Uh oh!

pllim Oct 11, 2021

Choose a reason for hiding this comment

Uh oh!

WilliamJamieson Oct 11, 2021

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

WilliamJamieson commented Oct 5, 2021 •

edited by pllim

Loading