Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Address Ruff rule PLR0911 in io #16097

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Address Ruff rule PLR0911 in io #16097

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 0 additions & 1 deletion .ruff.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -250,7 +250,6 @@ lint.unfixable = [
"astropy/io/*" = [
"G001", # logging-string-format
"G004", # logging-f-string
"PLR0911", # too-many-return-statements
"PTH116", # os-stat
"PTH117", # os-path-isabs
"RET505", # superfluous-else-return
Expand Down
26 changes: 9 additions & 17 deletions astropy/io/fits/card.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1272,29 +1272,21 @@ def _format_value(value):
if isinstance(value, str):
if value == "":
return "''"
else:
exp_val_str = value.replace("'", "''")
val_str = f"'{exp_val_str:8}'"
return f"{val_str:20}"
exp_val_str = value.replace("'", "''")
val_str = f"'{exp_val_str:8}'"
return f"{val_str:20}"

val_str = None
# must be before int checking since bool is also int
elif isinstance(value, (bool, np.bool_)):
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is an example of a rule forcing worse code style - since every branch has an immediate return, what was there was super clear. Not enough to nix this, but it adds to my feeling that we're starting to apply rules where common sense would be a better guide.

I think a change like you make below, where every branch that has a return has an if instead of an elif would be a better improvement.

Copy link
Member Author

Choose a reason for hiding this comment

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

The one argument in favor of having a single return statement is that all the strings are formatted with :>20 and the single return statement allows not having to repeat that.

Copy link
Contributor

Choose a reason for hiding this comment

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

That's a fair argument! As I noted above, I don't feel strongly enough to nix the PR for it, but worried we were letting rules force styles. Given that you made the changes, I'm happy to go with them - would just suggest not to start enforcing the rule (and thus not have noqa for it).

return f"{repr(value)[0]:>20}" # T or F

if isinstance(value, (bool, np.bool_)):
val_str = repr(value)[0] # T or F
elif _is_int(value):
return f"{value:>20d}"

val_str = str(value)
elif isinstance(value, (float, np.floating)):
return f"{_format_float(value):>20}"

val_str = _format_float(value)
elif isinstance(value, (complex, np.complexfloating)):
val_str = f"({_format_float(value.real)}, {_format_float(value.imag)})"
return f"{val_str:>20}"

elif isinstance(value, Undefined):
return ""
else:
return ""
return "" if val_str is None else f"{val_str:>20}"


def _format_float(value):
Expand Down
115 changes: 50 additions & 65 deletions astropy/io/fits/column.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1364,74 +1364,59 @@

return format, recformat

def _convert_to_valid_data_type(self, array):
def _convert_to_valid_data_type(self, array): # noqa: PLR0911
Copy link
Contributor

Choose a reason for hiding this comment

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

This noqa just makes the code less clear for most devs who will have no clue what this means.

# Convert the format to a type we understand
if isinstance(array, Delayed):
if isinstance(array, Delayed) or array is None:
return array
elif array is None:
format = self.format
Copy link
Contributor

Choose a reason for hiding this comment

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

This is definitely for the better.

dims = self._dims
if dims and format.format not in "PQ":
shape = dims[:-1] if "A" in format else dims
shape = (len(array),) + shape
array = array.reshape(shape)

if "P" in format or "Q" in format:
return array
else:
format = self.format
dims = self._dims
if dims and format.format not in "PQ":
shape = dims[:-1] if "A" in format else dims
shape = (len(array),) + shape
array = array.reshape(shape)

if "P" in format or "Q" in format:
return array
elif "A" in format:
if array.dtype.char in "SU":
if dims:
# The 'last' dimension (first in the order given
# in the TDIMn keyword itself) is the number of
# characters in each string
fsize = dims[-1]
else:
fsize = np.dtype(format.recformat).itemsize
return chararray.array(array, itemsize=fsize, copy=False)
else:
return _convert_array(array, np.dtype(format.recformat))
elif "L" in format:
# boolean needs to be scaled back to storage values ('T', 'F')
if array.dtype == np.dtype("bool"):
return np.where(array == np.False_, ord("F"), ord("T"))
else:
return np.where(array == 0, ord("F"), ord("T"))
elif "X" in format:
return _convert_array(array, np.dtype("uint8"))
else:
# Preserve byte order of the original array for now; see #77
numpy_format = array.dtype.byteorder + format.recformat

# Handle arrays passed in as unsigned ints as pseudo-unsigned
# int arrays; blatantly tacked in here for now--we need columns
# to have explicit knowledge of whether they treated as
# pseudo-unsigned
bzeros = {
2: np.uint16(2**15),
4: np.uint32(2**31),
8: np.uint64(2**63),
}
if (
array.dtype.kind == "u"
and array.dtype.itemsize in bzeros
and self.bscale in (1, None, "")
and self.bzero == bzeros[array.dtype.itemsize]
):
# Basically the array is uint, has scale == 1.0, and the
# bzero is the appropriate value for a pseudo-unsigned
# integer of the input dtype, then go ahead and assume that
# uint is assumed
numpy_format = numpy_format.replace("i", "u")
self._pseudo_unsigned_ints = True

# The .base here means we're dropping the shape information,
# which is only used to format recarray fields, and is not
# useful for converting input arrays to the correct data type
dtype = np.dtype(numpy_format).base

return _convert_array(array, dtype)
if "A" in format:
if array.dtype.char not in "SU":
return _convert_array(array, np.dtype(format.recformat))

Check warning on line 1382 in astropy/io/fits/column.py

View check run for this annotation

Codecov / codecov/patch

astropy/io/fits/column.py#L1382 

Added line #L1382 was not covered by tests
# The 'last' dimension (first in the order given
# in the TDIMn keyword itself) is the number of
# characters in each string
fsize = dims[-1] if dims else np.dtype(format.recformat).itemsize
return chararray.array(array, itemsize=fsize, copy=False)
if "L" in format:
Copy link
Contributor

Choose a reason for hiding this comment

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

I like the pattern of if ...: ... return as being very clear that this is a piece of code that just exists.

Might add an empty line in between myself, but really this is nicer than what was there.

# boolean needs to be scaled back to storage values ('T', 'F')
return np.where(array == 0, ord("F"), ord("T"))
if "X" in format:
return _convert_array(array, np.dtype("uint8"))
# Preserve byte order of the original array for now; see #77
numpy_format = array.dtype.byteorder + format.recformat

# Handle arrays passed in as unsigned ints as pseudo-unsigned
# int arrays; blatantly tacked in here for now--we need columns
# to have explicit knowledge of whether they treated as
# pseudo-unsigned
bzeros = {2: np.uint16(2**15), 4: np.uint32(2**31), 8: np.uint64(2**63)}
if (
array.dtype.kind == "u"
and array.dtype.itemsize in bzeros
and self.bscale in (1, None, "")
and self.bzero == bzeros[array.dtype.itemsize]
):
# Basically the array is uint, has scale == 1.0, and the
# bzero is the appropriate value for a pseudo-unsigned
# integer of the input dtype, then go ahead and assume that
# uint is assumed
numpy_format = numpy_format.replace("i", "u")
self._pseudo_unsigned_ints = True

# The .base here means we're dropping the shape information,
# which is only used to format recarray fields, and is not
# useful for converting input arrays to the correct data type
dtype = np.dtype(numpy_format).base

return _convert_array(array, dtype)


class ColDefs(NotifierMixin):
Expand Down
8 changes: 3 additions & 5 deletions astropy/io/fits/hdu/hdulist.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1281,14 +1281,12 @@ def _read_next_hdu(self):

Returns True if a new HDU was loaded, or False otherwise.
"""
if self._read_all:
fileobj = self._file
if self._read_all or (fileobj is not None and fileobj.closed):
return False

saved_compression_enabled = compressed.COMPRESSION_ENABLED
fileobj, data, kwargs = self._file, self._data, self._open_kwargs

if fileobj is not None and fileobj.closed:
return False
data, kwargs = self._data, self._open_kwargs

try:
self._in_read_next_hdu = True
Expand Down