astropy
CLN: cleanup `Model.n_inputs` and `Model.n_outputs` properties
Description
This is a follow up to #9298, and cleans up code that was introduced in 4.0 to mitigate a deprecation that was concluded in 5.0
For context, I discovered this while working on #16630, from which this is extracted.
- [ ] 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.
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[bot] avatar](https://avatars.githubusercontent.com/in/15368?v=4 "Published by a pub.dev verified publisher"){kind=small}
👋 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?
My approach isn't quite right: removing these properties altogether makes the instance attribute writable when it clearly shouldn't be.
I should note that I revised this PR since my previous comment. This is now ready for review.
I feel like this needs a change log because some "deprecated behavior" is now removed but let's wait for modeling maintainers to chime in. Thanks!
Supposedly it's just a clean up and shouldn't be visible to users, but I'll admit this was pretty confusing for me so maybe I misunderstood the implications, in which case I'll happily add a changelog entry !
It's probably worth adding a change log since it is removing things.
Is it ? from a user perspective I don't know that anything is going away, or what the changelog should say.
I think there should be a changelog for people who write their own models, they don't need to define these properties any more?
My understanding is they already didn't.
It seems this patch is more confusing than I anticipated so let me try to explain what I did a little better:
The base Model class currently defines n_inputs and n_outputs as class attributes and then rebinds these names to properties. This means that the class attributes are shadowed immediately and thus never observable for end users. This is why ruff 0.5 is flagging them (rule F811), so removing them doesn't affect users.
While I was in there, I saw these TODO comments indicating that complexity in these properties was meant to be eliminated so I followed the bread crumbs and removed everything that wasn't covered by tests, but again, nothing I did there is visible to users.
I need to review this and run tests. By design the class attributes n_inputs and n_outputs are the only way to set the number of inputs and outputs, as inputs and outputs can be changed. However, there are some models (Mapping, Identity) for which the number of in(out)puts can be determined only at the instance level. IIRC, a work around this was to introduce the properties.
Setting n_inputs and n_outputs in new models is required. I'm hesitant to approve this without a careful review and as a minimum it will require documentation changes.
Are we good to merge this so https://github.com/astropy/astropy/pull/16630 can go in as well?
@nstarman I've added noqa comments in #16630 and linked to this PR so #16630 is not blocked by this one anymore.
@nden there's no rush on my side. I see that the likelihood that I'm the one who's most confused by this change is skyrocketing, but it's still not clear to me that this has any user-visible effects; any visible API change here is unintentional and suffices to justify closing this PR without merge.
we never converged on wether this needs a changelog or not. @Cadair argued there should be one (https://github.com/astropy/astropy/pull/16633#issuecomment-2213618651) but it's still not clear to me that this has any user visible implications (https://github.com/astropy/astropy/pull/16633#issuecomment-2197563387). In any case, this is not meant to have any externally observable impact, so as much as I'd fancy writing a changelog just to expedite the PR, I can't do so without a clear understanding of what needs to be told. If anyone insists that there's a user visible change here, it'd help a lot if I could see an example of broken code.
On the off chance that this is actually breaking to anyone (but I still do not think it is), how would we feel about including this in the first release candidate for 7.0 ? if anyone reports anything we should just revert it then.
I think your explanation makes sense, so I am not going to insist on a changelog. Happy to ship this in 7.0rc and see what happens.
Can u pls rebase again just to make sure CI still green? Wheel jobs got stuck. Thanks!