Skip to content

Updates to ~astropy.modeling.custom_model#11984

Merged
nden merged 2 commits intoastropy:mainfrom
WilliamJamieson:feature/custom_model_refactor
Aug 13, 2021
Merged

Updates to ~astropy.modeling.custom_model#11984
nden merged 2 commits intoastropy:mainfrom
WilliamJamieson:feature/custom_model_refactor

Conversation

@WilliamJamieson
Copy link
Copy Markdown
Contributor

@WilliamJamieson WilliamJamieson commented Jul 28, 2021 •
edited by nden
Loading

Description

This pull request addresses the issues raised in #11791. Namely, what should be done with "parameters" set using custom_model which directly relate to the internal workings of Model.

In particular, it pointed out that weird behavior occurs when the user tries to set n_outputs via the custom_model interface. This exposed the fact that custom_model assumed that the model generated would always have exactly one output value, which should not be the case.

This PR addresses the question of the banned/reserved list of parameters, by banning use of parameters which share a name with any "non-settable" property used in Model and using parameters sharing a name with a "settable" property to set the underlying Model property. It also introduces the notion of a special parameter, which is a model parameter which is not settable, but maybe one that users may want to manipulate when dynamically creating new models (e.g. n_outputs).

Fixes #11791

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.

@WilliamJamieson
Copy link
Copy Markdown
Contributor Author

WilliamJamieson commented Jul 28, 2021

Tagging @nden and @perrygreenfield.

@pllim pllim added this to the v5.0 milestone Jul 28, 2021
@pllim pllim requested review from nden and perrygreenfield July 28, 2021 15:49
@WilliamJamieson WilliamJamieson mentioned this pull request Jul 28, 2021
Closed
@WilliamJamieson WilliamJamieson force-pushed the feature/custom_model_refactor branch from b3e78ed to e8ceecd Compare July 28, 2021 16:43
nden
nden approved these changes Aug 13, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@nden nden left a comment

Choose a reason for hiding this comment

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

LGTM

@nden
Copy link
Copy Markdown
Contributor

nden commented Aug 13, 2021

@WilliamJamieson Can you add a few words at the bottom of the first paragraph in this page
https://docs.astropy.org/en/stable/modeling/new-model.html
and mention the option to have more than one output.

@nden
Copy link
Copy Markdown
Contributor

nden commented Aug 13, 2021

@dstansby Does this solve the issue with custom_model you reported ?

@dstansby
Copy link
Copy Markdown
Contributor

dstansby commented Aug 13, 2021

Thanks for opening this - looking through the tests it looks like it solves my problem 👍

@WilliamJamieson WilliamJamieson force-pushed the feature/custom_model_refactor branch from 0853501 to 19bfd4c Compare August 13, 2021 16:59
nden
nden reviewed Aug 13, 2021
View reviewed changes
@WilliamJamieson WilliamJamieson force-pushed the feature/custom_model_refactor branch 2 times, most recently from 20044ec to 8f18522 Compare August 13, 2021 17:42
@nden
Copy link
Copy Markdown
Contributor

nden commented Aug 13, 2021

@pllim Is the failure known? It looks unrelated. If so feel free to merge.

@pllim
Copy link
Copy Markdown
Member

pllim commented Aug 13, 2021

It is because of #12050 but unless you are in a big hurry, I would advise to wait till we fix the CI in main and then you can rebase.

@pllim
Copy link
Copy Markdown
Member

pllim commented Aug 13, 2021

Thanks to @nstarman , it should be fixed now via #12052 . Please rebase and let the CI rerun. Thanks!

@WilliamJamieson
Changes to enable more fine-grained custom_models
f0cc46b 
Added tests for updates to `custom_model`.

Added some documentation for the changes

Added changelog entry
@WilliamJamieson
Added the requested documentation.
087d1fa
@WilliamJamieson WilliamJamieson force-pushed the feature/custom_model_refactor branch from 8f18522 to 087d1fa Compare August 13, 2021 20:00
@nden nden merged commit d70876e into astropy:main Aug 13, 2021
@WilliamJamieson WilliamJamieson deleted the feature/custom_model_refactor branch August 13, 2021 22:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@nden nden nden approved these changes

@perrygreenfield perrygreenfield Awaiting requested review from perrygreenfield

Assignees

No one assigned

Labels

modeling Refactoring

Projects

None yet

Milestone

v5.0

Development

Successfully merging this pull request may close these issues.

What to do when n_outputs is given to custom_model?

4 participants

@WilliamJamieson @nden @dstansby @pllim