Skip to content

Add has_tied, has_fixed and has_bounds properties to Model#16677

Merged
pllim merged 3 commits intoastropy:mainfrom
astrofrog:modeling-has-constraints
Jul 8, 2024
Merged

Add has_tied, has_fixed and has_bounds properties to Model#16677
pllim merged 3 commits intoastropy:mainfrom
astrofrog:modeling-has-constraints

Conversation

@astrofrog
Copy link
Member

@astrofrog astrofrog commented Jul 4, 2024 •
edited
Loading

This has been split out from #16673 for ease of review

Models include tied, fixed, and bounds properties that return dictionaries with a summary of which parameters have constraints. However, sometimes one wants to quickly know whether there are any specified tied, fixed, or bounds constraints, and the main way to do that is:

 any(model.fixed.values())
 any(model.tied.values())
 any(b != (None, None) for b in model.bounds.values())

These appear notably in the fitting code - this turns out to be reasonably expensive operations especially in the fitting code, where they might be called every time the objective function is called, and it is also clunky to repeat this logic several times in different places.

This PR adds:

Model.has_fixed
Model.has_tied
Model.has_bounds

which simplifies this. In addition, these properties, like fixed, tied, and bounds are cached and the cached version is used when sync_constraints is False (typically during fitting), removing any performance impact. But beyond this, these properties could be generically useful to users, similarly to how we have e.g. has_units, hence why I made these public.

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

@astrofrog astrofrog requested a review from nden July 4, 2024 20:45
@github-actions
Copy link
Contributor

github-actions bot commented Jul 4, 2024

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?
  • 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
Copy link
Contributor

github-actions bot commented Jul 4, 2024

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

@astrofrog astrofrog added this to the v7.0.0 milestone Jul 4, 2024
@astrofrog astrofrog requested a review from perrygreenfield July 4, 2024 20:47
@astrofrog astrofrog marked this pull request as ready for review July 4, 2024 20:47
@astrofrog astrofrog requested a review from a team as a code owner July 4, 2024 20:47
@astrofrog astrofrog mentioned this pull request Jul 4, 2024
Merged
1 task
@astrofrog astrofrog requested a review from WilliamJamieson July 5, 2024 09:39
@astrofrog astrofrog force-pushed the modeling-has-constraints branch from 2b2560f to 2f33978 Compare July 5, 2024 21:26
perrygreenfield
perrygreenfield approved these changes Jul 8, 2024
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 pllim merged commit 14ff60e into astropy:main Jul 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@perrygreenfield perrygreenfield perrygreenfield approved these changes

@nden nden Awaiting requested review from nden

@WilliamJamieson WilliamJamieson Awaiting requested review from WilliamJamieson

Assignees

No one assigned

Labels

Enhancement modeling Ready-for-final-review

Projects

None yet

Milestone

v7.0.0

Development

Successfully merging this pull request may close these issues.

3 participants

@astrofrog @perrygreenfield @pllim