Skip to content

Allow Path in reproject_from_healpix - #625

Open
AlecThomson wants to merge 2 commits into
astropy:mainfrom
AlecThomson:main
Open

Allow Path in reproject_from_healpix#625
AlecThomson wants to merge 2 commits into
astropy:mainfrom
AlecThomson:main

Conversation

@AlecThomson

Copy link
Copy Markdown

The docs of reproject_from_healpix state:

def reproject_from_healpix(
    input_data, output_projection, shape_out=None, hdu_in=1, order="bilinear", nested=None, field=0
):
    """
    Reproject data from a HEALPIX projection to a standard projection.

    Parameters
    ----------
    input_data : object
        The input data to reproject. This can be:

            * The name of a HEALPIX FITS file
            * A `~astropy.io.fits.TableHDU` or `~astropy.io.fits.BinTableHDU`
              instance
            * A tuple where the first element is a `~numpy.ndarray` and the
              second element is a `~astropy.coordinates.BaseCoordinateFrame`
              instance or a string alias for a coordinate frame.

However, in the current first branch is only checked in 'The name of a HEALPIX FITS file' is a str type. This is a simple PR to fix the type checking for input_data.

@codecov

codecov Bot commented Jul 20, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.62%. Comparing base (a8a2eb4) to head (227f1bf).

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #625   +/-   ##
=======================================
  Coverage   90.62%   90.62%           
=======================================
  Files          51       51           
  Lines        2431     2433    +2     
=======================================
+ Hits         2203     2205    +2     
  Misses        228      228           

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

if isinstance(input_data, str | Path):
with fits.open(input_data) as hdulist:
return parse_input_healpix_data(hdulist[hdu_in or 1], field=field)
if isinstance(input_data, tuple) and isinstance(input_data[0], np.ndarray):

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.

Just curious, why was this changed from elif to if? (and same above)

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

This was a tiny bit of refactoring on my part. The previous state was very close to a series of guard clauses, so I formulated it more directly that way. I think the logic remains identical. Following a guard clause style, the early returns mean we don't have to worry about the else portions from each previous check.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Something that did just catch my attention now is that the final type error isn't really complete, and it doesn't really tell the caller what ended up there. I'd be tempted to make it something like:

msg = (
    "input_data should be an HDU object, a filename, or a tuple "
    f"of (array, frame), got {type(input_data)=}"
)
raise TypeError(msg)

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

FWIW - there's even a Ruff rule that enforces this
https://docs.astral.sh/ruff/rules/superfluous-else-return/

Which is maybe why I've picked up the habit!

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.

Feel free to adjust the error message - your suggestion would be an improvement

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants