Model evaluation refactor by WilliamJamieson · Pull Request #11931 · astropy/astropy

WilliamJamieson · 2021-07-08T22:17:41Z

Description

This pull request extends the refactoring work in PR #11930 (indeed it depends on this PR so it should be rebased after this PR is merged) by further cleaning up model evaluation methods in ~astropy.modeling.core. This is an attempt to remove repeated code while moving methods closer to where they are used.

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.
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.

github-actions · 2021-07-08T22:18:17Z

👋 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?

pep8speaks · 2021-07-15T19:15:22Z

Hello @WilliamJamieson 👋! It looks like you've made some changes in your pull request, so I've checked the code again for style.

There are no PEP8 style issues with this pull request - thanks! 🎉

Comment last updated at 2021-10-25 18:47:29 UTC

Integrated new bounding_box into automatic bounding_boxes Finished TODO to remove check on bounding box input processing. Updated _fix_input_bounding_box to be in line with PR astropy#11908. Made fix_inputs_with_bounding_box more robust Improved test_custom_model_bounding_box test to test with_bounding_box=True Removed debugging print from a test. Added changelog entry. Added `__all__` to bounding_box.py Fixed more docstrings Integrated new bounding_box into automatic bounding_boxes Finished TODO to remove check on bounding box input processing. Improved test_custom_model_bounding_box test to test with_bounding_box=True Removed debugging print from a test. Added `__all__` to bounding_box.py Fixed more docstrings Refactored generic_call Removed generic_call from core.py Refactored _generic_evaluate into Model class Moved get_bounding_box into model class Moved _prepare_inputs_* functions into model class Refactored _prepare_outputs_* into model. Refactored validate_input_shapes into model Refactored some helper functions into model. Fixed codestyle errors. Added changelog entry. Minor refactoring and additional documentation. More documentation added. Fixed get_bounding_box in asdf Moved changelog to correct folder Fixed rebase issue Refactored and added tests to increase test coverage.

perrygreenfield

Just some minor comments; otherwise LGTM

perrygreenfield · 2021-10-25T17:35:09Z

+
+        return output
+
+    def _prepare_outputs_single_model(self, outputs, format_info):


This might be a good opportunity to change the awful name format_info

Though I wouldn't want to hold up getting this into the next astropy release. A new issue?

Agreed, but I could not come up with a better name at the time. Maybe broadcast_shapes?

Or broadcasted_shapes? Which implies an action taken upon them.

perrygreenfield · 2021-10-25T17:53:19Z

+            else:
+                try:
+                    return output.reshape(broadcast_shape)
+                except ValueError:


Some explanation of why this differs from the previous version would be helpful in the review. E.g., what previous problem is this fixing, or new capability it now handles. It otherwise seems more complex than the previous logic as well as relying on exception handling.

This bit (of convoluted logic) is literally the old _prepare_outputs_single_model function. I just moved it from being a function to a bound static method, so that it was closer in the code to where it was actually being used.

I got the heritage and the movement into methods, where the first arguments were essentially self (in some cases literally!); I was mainly asking about the new logic structure.

This is somewhat explained in this comment. The major goal was to refactor the bounding box related parts of model evaluation so that they were performed by the bounding box itself. This paves the way for the CompoundBoundingBox in #11942, to add additional bounding box logic without lots of ugly hacks in core.py.

The rest of the logic structure was my attempt to move all the evaluation inside Model itself in such a way that CompoundModel could modify that logic as a sub class of Model instead of a lot of scattered external logic. Doing this made it easier to move the bounding box logic out in a consistent fashion.

WilliamJamieson · 2021-10-25T19:09:49Z

@perrygreenfield. This PR is intended to streamline the model evaluation code (in my opinion). It moves the code from functions which "look-like" they should be bound methods but are not bound methods, to actual bound methods. So a lot of the changes are cut and pasting code around core.py with minor changes to interfaces. In the end means there is less overall duplicated logic in model evaluation between Model and it's subclass CompoundModel. In my opinion these changes make it easier to trace the flow of the program through the steps of an evaluation.

Moreover, since the bounding_box has become a more functional object. All of the bounding box enforcement logic has been moved into the BoundingBox class. The move of the bounding_box related parts of evaluation was also done with an eye towards the CompoundBoundingBox feature which is being introduced in PR #11942, so that bounding box enforcement in that case would be transparent to the user.

The main goal of this refactor, in my mind, was to ready modeling for future refactoring of models so that core.py ultimately becomes less complex and more modular. There are enough changes after just this re-arrangement that I figured it should be reviewed at this point.

perrygreenfield

updated comments

perrygreenfield · 2021-10-25T19:46:10Z

+
+        return output
+
+    def _prepare_outputs_single_model(self, outputs, format_info):


Or broadcasted_shapes? Which implies an action taken upon them.

perrygreenfield · 2021-10-25T19:51:44Z

+            else:
+                try:
+                    return output.reshape(broadcast_shape)
+                except ValueError:


I got the heritage and the movement into methods, where the first arguments were essentially self (in some cases literally!); I was mainly asking about the new logic structure.

nden · 2021-10-26T01:51:15Z

@pllim Is there an issue with this going into 5.0?

pllim · 2021-10-26T12:01:07Z

No. I wasn't expecting it to be merged so soon. This is not the astropy way. 😂

Will readjust milestone.

perrygreenfield · 2021-10-26T12:02:31Z

My fault then. I was under the impression this was urgent to get in for the release (and there is another PR dependent on it)

pllim · 2021-10-26T12:05:39Z

I did add a commit on main (7710c82) to fix the change log. This is strictly speaking not a new feature, but closer to API change. Also, you don't use ~ in double-backticks; it does not do anything.

pllim · 2021-10-26T12:05:59Z

My fault then

No one's fault!

nden · 2021-10-26T14:41:56Z

Cool. Thanks for figuring this out!

github-actions Bot added zzz 💤 io.misc.asdf archived: Go to asdf-astropy repo modeling labels Jul 8, 2021

WilliamJamieson mentioned this pull request Jul 13, 2021

Adding CompoundBoundingBox #11942

Merged

9 tasks

WilliamJamieson marked this pull request as ready for review July 13, 2021 20:41

WilliamJamieson force-pushed the feature/model_evaluation_refactor branch 7 times, most recently from 10316de to 290fc4e Compare July 21, 2021 18:57

WilliamJamieson force-pushed the feature/model_evaluation_refactor branch 3 times, most recently from a46a3fd to 986445d Compare July 27, 2021 17:25

WilliamJamieson mentioned this pull request Jul 29, 2021

Fixed possible race condition in parallel testing #11972

Merged

9 tasks

WilliamJamieson force-pushed the feature/model_evaluation_refactor branch from 986445d to 38642ef Compare July 30, 2021 15:35

WilliamJamieson force-pushed the feature/model_evaluation_refactor branch from 38642ef to 1c2dfd1 Compare September 17, 2021 20:08

WilliamJamieson force-pushed the feature/model_evaluation_refactor branch from 1c2dfd1 to a44bea7 Compare October 20, 2021 19:30

WilliamJamieson mentioned this pull request Oct 22, 2021

Enable Compound Bounding Box support spacetelescope/gwcs#375

Merged

WilliamJamieson force-pushed the feature/model_evaluation_refactor branch from a44bea7 to 4a4fbc9 Compare October 22, 2021 18:37

WilliamJamieson force-pushed the feature/model_evaluation_refactor branch from 4a4fbc9 to 3b6f4df Compare October 25, 2021 18:47

perrygreenfield reviewed Oct 25, 2021

View reviewed changes

pllim added this to the v5.1 milestone Oct 25, 2021

pllim added the Refactoring label Oct 25, 2021

perrygreenfield reviewed Oct 25, 2021

View reviewed changes

perrygreenfield approved these changes Oct 25, 2021

View reviewed changes

perrygreenfield merged commit 0de9903 into astropy:main Oct 25, 2021

pllim modified the milestones: v5.1, v5.0 Oct 26, 2021

pllim added a commit that referenced this pull request Oct 26, 2021

DOC: Fix change log of #11931

7710c82

WilliamJamieson deleted the feature/model_evaluation_refactor branch October 26, 2021 13:05

This was referenced Oct 28, 2021

Changed format_info variable name to broadcasted_shape in modeling #12314

Merged

Error in bounding box when running with mixed scalar and array arguments. #12319

Closed


		return output

		def _prepare_outputs_single_model(self, outputs, format_info):

Uh oh!

Uh oh!

Conversation

WilliamJamieson commented Jul 8, 2021

Description

Checklist for package maintainer(s)

Uh oh!

github-actions Bot commented Jul 8, 2021

Uh oh!

pep8speaks commented Jul 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Comment last updated at 2021-10-25 18:47:29 UTC

Uh oh!

perrygreenfield left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

WilliamJamieson commented Oct 25, 2021

Uh oh!

perrygreenfield left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nden commented Oct 26, 2021

Uh oh!

pllim commented Oct 26, 2021

Uh oh!

perrygreenfield commented Oct 26, 2021

Uh oh!

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

Uh oh!

Uh oh!

pllim commented Oct 26, 2021

Uh oh!

nden commented Oct 26, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

pep8speaks commented Jul 15, 2021 •

edited

Loading

WilliamJamieson Oct 25, 2021 •

edited

Loading

pllim commented Oct 26, 2021 •

edited

Loading