Skip to content

RFC: replace nested if/else logic tree with flat pattern matching in discretize_model - #17825

Merged
pllim merged 1 commit into
astropy:mainfrom
neutrinoceros:convolution/rfc/pattern_matching_distretize_model
Feb 28, 2025
Merged

RFC: replace nested if/else logic tree with flat pattern matching in discretize_model#17825
pllim merged 1 commit into
astropy:mainfrom
neutrinoceros:convolution/rfc/pattern_matching_distretize_model

Conversation

@neutrinoceros

@neutrinoceros neutrinoceros commented Feb 27, 2025

Copy link
Copy Markdown
Contributor

Description

Following a suggestion from @pllim in https://github.com/astropy/astropy/pull/17823/files#r1971954084, I was curious to see if it helped readibility, and I think it does, so, submitting.

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

@neutrinoceros neutrinoceros added this to the v7.1.0 milestone Feb 27, 2025
@neutrinoceros neutrinoceros mentioned this pull request Feb 27, 2025
Merged
1 task done
@github-actions

github-actions Bot commented Feb 27, 2025

Copy link
Copy Markdown
Contributor

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.

@neutrinoceros
neutrinoceros force-pushed the convolution/rfc/pattern_matching_distretize_model branch from 0c67df0 to 015723a Compare February 27, 2025 17:13
@neutrinoceros

neutrinoceros commented Feb 27, 2025

Copy link
Copy Markdown
Contributor Author

Note that it actually fixes a bug of sorts: previously the return value may be None when ndim was neither 1 or 2, but it's documented as an uncondtional ndarray, and we don't seem to have any test expecting a None there. So, if that behavior was actually desired, it should be documented and tested. Otherwise, this patch fixes the issue.

@neutrinoceros
neutrinoceros force-pushed the convolution/rfc/pattern_matching_distretize_model branch from 015723a to 644cf40 Compare February 27, 2025 17:21
@neutrinoceros neutrinoceros changed the title RFC: replace nested if/else logic tree with flat pattern matching in discretize_model RFC: replace nested if/else logic tree with flat pattern matching in discretize_model Feb 27, 2025
@pllim
pllim requested a review from keflavich February 27, 2025 18:43
@pllim

pllim commented Feb 27, 2025

Copy link
Copy Markdown
Member

If this changes a look-up behavior, perhaps a bugfix change log? Thought might not want to backport.

@keflavich

keflavich commented Feb 27, 2025

Copy link
Copy Markdown
Contributor

I'm going to come across as old and conservative here, but this looks nothing like python to me and I don't know how to read it. I'll admit it's cleaner, briefer text, which I like, but it's a new language to me. I'll probably end up using it in a couple years. So, I'm :+0: on this.

Regarding the corner case - I don't think it can occur. ndim is set by

astropy/astropy/convolution/utils.py

Line 177 in 644cf40

ndim = model.n_inputs

i.e., it is the model dimensionality, and ndim > 2 is already checked for:

astropy/astropy/convolution/utils.py

Line 178 in 644cf40

if ndim > 2:

the only other case to consider is ndim <= 0, which I think should not be possible (astropy doesn't have 0D models, right?).

@neutrinoceros

neutrinoceros commented Feb 28, 2025

Copy link
Copy Markdown
Contributor Author

the only other case to consider is ndim <= 0, which I think should not be possible (astropy doesn't have 0D models, right?).

Right now it doesn't, but the refactor still makes it more robust if this ever changed. In the old version, the function would just silently return None, which could be hard to debug, depending on how it manifests.

@neutrinoceros

neutrinoceros commented Feb 28, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

If this changes a look-up behavior, perhaps a bugfix change log? Thought might not want to backport.

As clarified by @keflavich, it doesn't change behavior in any existing case. It does preclude the introduction of a bug if we were to support 0D models1, and to some extent makes the following guard clause redundant (but keeping it doesn't seem to harm either), so I don't think we need a changelog at this point.

astropy/astropy/convolution/utils.py

Lines 178 to 179 in 644cf40

if ndim > 2:
raise ValueError("discretize_model supports only 1D and 2D models.")

I don't know how to read it.

Happy to explain any specific aspect that is not clear to you, btw.

Footnotes

  1. admittedly this is a silly idea. The real argument being that the proposed refactor makes this function more internally consistent, which is always preferable to relying on external implementation details IMHO. ↩

@keflavich

keflavich commented Feb 28, 2025

Copy link
Copy Markdown
Contributor

I agree, this refactor enforces consistency. Also, agree on no changelog needed.

pllim
pllim reviewed Feb 28, 2025
View reviewed changes
Comment thread astropy/convolution/utils.py
raise ValueError("Invalid mode for discretize_model.")
case _:
raise ValueError(
"Invalid (mode, ndim) combination for discretize_model. "

@pllim pllim Feb 28, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can we at least keep the error message starting out the same as the old message? In case downstream is doing a match in their test? No reason to change that?

@neutrinoceros neutrinoceros Feb 28, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I could write it as "Invalid mode, or ndim for discretize_model", so it would match two words instead of one, but then, what happens if someone tries to match the whole string, and where does it end ? IMO, this is why downstream has no business testing a message they do not control.

@pllim pllim Feb 28, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Ah right, the ndim... I tried to suggest something but it didn't flow right. Since Adam approved... let's keep what you have. Thanks!

@pllim

pllim commented Feb 28, 2025

Copy link
Copy Markdown
Member

@keflavich , with typing and so on, does Python even look like Python anymore? 🤪 The case matching is the one thing I miss switching from C and Python (typing not so much).

pllim
pllim approved these changes Feb 28, 2025
View reviewed changes
Comment thread astropy/convolution/utils.py
raise ValueError("Invalid mode for discretize_model.")
case _:
raise ValueError(
"Invalid (mode, ndim) combination for discretize_model. "

@pllim pllim Feb 28, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Ah right, the ndim... I tried to suggest something but it didn't flow right. Since Adam approved... let's keep what you have. Thanks!

@pllim
pllim merged commit a1f4955 into astropy:main Feb 28, 2025
@neutrinoceros
neutrinoceros deleted the convolution/rfc/pattern_matching_distretize_model branch February 28, 2025 15:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@keflavich keflavich keflavich approved these changes
@pllim pllim pllim approved these changes
@larrybradley larrybradley Awaiting requested review from larrybradley larrybradley is a code owner

Assignees

No one assigned

Labels

convolution no-changelog-entry-needed Refactoring

Projects

None yet

Milestone

v7.1.0

Development

Successfully merging this pull request may close these issues.

3 participants

@neutrinoceros @pllim @keflavich