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.

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

✅ Fix tests broken by #2058 #2059

Merged
merged 65 commits into from
Mar 5, 2024
Merged

✅ Fix tests broken by #2058 #2059

merged 65 commits into from
Mar 5, 2024

Conversation

shnizzedy
Copy link
Member

@shnizzedy shnizzedy commented Jan 30, 2024

Fixes

Fixes #2058 by @shnizzedy
Related to #2048 by @nx10

Description

  • Fixes CPAC/utils/__init__.py, which was broken by pre-commit modifications
  • Restores TSV and CSV files, which were broken by pre-commit modifications
  • Fixes CPAC/nuisance/utils/__init__.py, which was broken by pre-commit modifications
  • In autoversioning script, wait for git lock when git adding changed files
  • Move entrypoint scripts (run.py and run-with-freesurfer.sh) from /dev/docker_data to CPAC/_entrypoints
  • Upgrades print statements to log statements where such statements were relevant to tests
  • Moves

    C-PAC/CPAC/utils/utils.py

    Lines 591 to 612 in 2e368b9

    def check_random_state(seed):
    """
    Turn seed into a np.random.RandomState instance
    Code from scikit-learn (https://github.com/scikit-learn/scikit-learn).
    Parameters
    ----------
    seed : None | int | instance of RandomState
    If seed is None, return the RandomState singleton used by np.random.
    If seed is an int, return a new RandomState instance seeded with seed.
    If seed is already a RandomState instance, return it.
    Otherwise raise ValueError.
    """
    if seed is None or seed is np.random:
    return np.random.mtrand._rand
    if isinstance(seed, (numbers.Integral, np.integer)):
    return np.random.RandomState(seed)
    if isinstance(seed, np.random.RandomState):
    return seed
    raise ValueError(
    "%r cannot be used to seed a numpy.random.RandomState" " instance" % seed
    )
    (which was lifted from scikit-learn) to
    def check_random_state(seed: Union[None, int, RandomState]) -> RandomState:
    """Turn seed into a np.random.RandomState instance.
    Parameters
    ----------
    seed : None | int | instance of RandomState
    If seed is None, return the RandomState singleton used by np.random.
    If seed is an int, return a new RandomState instance seeded with seed.
    If seed is already a RandomState instance, return it.
    Otherwise raise ValueError.
    """
    if seed is None or seed is np.random:
    return np.random.mtrand._rand
    if isinstance(seed, (numbers.Integral, np.integer)):
    return np.random.RandomState(seed)
    if isinstance(seed, np.random.RandomState):
    return seed
    msg = f"{seed!r} cannot be used to seed a numpy.random.RandomState instance"
    raise ValueError(msg)
    for clearer licensing and provenance
  • Replaced sets.Set with set, which has been built-in since Python 2.4 (November 30, 2004)!
  • Deprecates repickle
  • Linted cwas files involved in what were failing tests
  • Style changes

Technical details

CPAC/utils/__init__.py

  • Adds used public symbols to CPAC.utils.__init__.__all__
  • Removes unused public symbols from CPAC.utils
  • Deprecates repickle and lints CPAC.utils.docs while modifying those sections of code

TSVs and CSVs

  • Restores TSVs and CSVs from before pre-commit modifications
  • Excludes TSVs from trailing-whitespace hook
  • Excludes CSVs, pickles and TSVs from end-of-file-fixer hook

CPAC/nuisance/utils/__init__.py

  • Moves class and function definitions from CPAC/nuisance/utils/__init__.py to CPAC/nuisance/utils/utils.py
  • Imports used public symbols in CPAC/nuisance/utils/__init__.py and adds them to CPAC.utils.__init__.__all__
  • Lints both of those files

print statements → log statements

Deprecates repickle

  • Instead of linting this function and upgrading its print statements, I thought we could just deprecate it, for the reason given in the deprecation message:

    C-PAC/CPAC/utils/utils.py

    Lines 1327 to 1331 in 4650f4f

    @deprecated(
    "1.8.7",
    "Python 2's end of life was over 4 years prior to this release. A user jumping from a C-PAC version that used Python 2 can use this function in any C-PAC version from 1.6.2 up until its removal in an upcoming version.",
    )
    def repickle(directory): # noqa: T20

    If we don't want this function deprecated, we can upgrade the function and its tests before we cut 1.8.7.

  • TODO: 📝 Document SOP for deprecating code fcp-indi.github.io#320

Linting some cwas files

  • The relevant change that was breaking the tests was ruff turned
    volume[np.where(mask_data == True)] = data
    into
    volume[np.where(mask_data is True)] = data
    , but np.True_ (array True from NumPy) ≠ True (scalar True from Python), so that broke the reshaping.
    volume[mask_data] = data
    is more Pythonic and equivalent to the original, so that's what is in this PR
  • Linted this file and the two test files that caught this error
  • Will replace other == True / == False / is True / is Falses in a subsequent PR

Style changes

order_by_type

  • I think order_by_type was set to true because I messed up translating my style suggestions here
  • If there's opposition to this style change, we can just revert 6c9febb...3b1bacf

flake8-errmsg (EM), f-string-missing-placeholders (F541), f-string (UP032)

  • Exception must not use (a string literal|an f-string literal|a .format() string directly), assign to variable first

  • f-string without any placeholders

  • Use f-string instead of format call

  • If there's opposition to this style change, we can just revert 9847b2c...2e368b9

Tests

All the tests that were failing in #2058 are passing here

Checklist

  • My pull request has a descriptive title (not a vague title like Update index.md).
  • My pull request targets the intial-run/ruff branch of the repository.
  • My commit messages follow best practices.
  • My code follows the established code style of the repository.
  • I added tests for the changes I made (n/a).
  • I updated the changelog (n/a).
  • I added or updated documentation.
  • I tried running the project locally and verified that there are no visible errors.

Developer Certificate of Origin

Developer Certificate of Origin
Developer Certificate of Origin
Version 1.1

Copyright (C) 2004, 2006 The Linux Foundation and its contributors.
1 Letterman Drive
Suite D4700
San Francisco, CA, 94129

Everyone is permitted to copy and distribute verbatim copies of this
license document, but changing it is not allowed.


Developer's Certificate of Origin 1.1

By making a contribution to this project, I certify that:

(a) The contribution was created in whole or in part by me and I
    have the right to submit it under the open source license
    indicated in the file; or

(b) The contribution is based upon previous work that, to the best
    of my knowledge, is covered under an appropriate open source
    license and I have the right under that license to submit that
    work with modifications, whether created in whole or in part
    by me, under the same open source license (unless I am
    permitted to submit under a different license), as indicated
    in the file; or

(c) The contribution was provided directly to me by some other
    person who certified (a), (b) or (c) and I have not modified
    it.

(d) I understand and agree that this project and the contribution
    are public and that a record of the contribution (including all
    personal information I submit with it, including my sign-off) is
    maintained indefinitely and may be redistributed consistent with
    this project or the open source license(s) involved.

@shnizzedy shnizzedy self-assigned this Jan 31, 2024
Comment on lines -1017 to -1061
def get_tr(tr):
"""Method to return TR in seconds."""
import re

if tr:
tr = re.search(r"\d+.\d+", str(tr)).group(0)
tr = float(tr)
if tr > 10:
tr = tr / 1000.0
else:
tr = ""
return tr


def check_tr(tr, in_file):
# imageData would have to be the image data from the funcFlow workflow,
# funcFlow outputspec.subject
import nibabel as nib

img = nib.load(in_file)

# get header from image data, then extract TR information, TR is fourth
# item in list returned by get_zooms()
imageHeader = img.header
imageZooms = imageHeader.get_zooms()
header_tr = imageZooms[3]

# If the TR information from header_tr (funcFlow) and convert_tr node
# (TR from config file) do not match, prepare to update the TR information
# from either convert_tr or header_tr using afni 3drefit, then append to
# func_to_mni
if header_tr != tr:
if tr is not None and tr != "":
TR = tr
else:
TR = header_tr

import warnings

warnings.warn(
"Warning: The TR information does not match between "
"the config and subject list files."
)

return TR
Copy link
Member Author

Choose a reason for hiding this comment

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

These weren't used anywhere, so I deleted them.

Comment on lines -148 to -174
# also write out volumes as individual files
# split = pe.Node(interface=fsl.Split(), name='split_raw_volumes_sca')
# split.inputs.dimension = 't'
# split.inputs.out_base_name = 'sca_'

# get_roi_num_list = pe.Node(util.Function(input_names=['timeseries_file',
# 'prefix'],
# output_names=['roi_list'],
# function=get_roi_num_list),
# name='get_roi_num_list')
# get_roi_num_list.inputs.prefix = "sca"

# sca.connect(inputNode, 'timeseries_one_d', get_roi_num_list,
# 'timeseries_file')

# rename_rois = pe.MapNode(interface=util.Rename(), name='output_rois',
# iterfield=['in_file', 'format_string'])
# rename_rois.inputs.keep_ext = True

# sca.connect(split, 'out_files', rename_rois, 'in_file')
# sca.connect(get_roi_num_list, 'roi_list', rename_rois, 'format_string')

sca.connect(corr, "out_file", concat, "in_files")
# sca.connect(concat, 'out_file', split, 'in_file')
sca.connect(concat, "out_file", outputNode, "correlation_stack")
# sca.connect(rename_rois, 'out_file', outputNode,
# 'correlation_files')
Copy link
Member Author

Choose a reason for hiding this comment

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

I think if we want to resurrect this functionality, we can rewrite it at least as easily as we could revive it from the commented-out code, so I deleted the commented-out code.

Comment on lines -494 to -531
def get_roi_num_list(timeseries_file, prefix=None):
# extracts the ROI labels from the 3dROIstats output CSV file
with open(timeseries_file, "r") as f:
roi_file_lines = f.read().splitlines()

roi_err = (
"\n\n[!] The output of 3dROIstats, used in extracting the "
"timeseries, is either empty, or not in the expected "
"format.\n\nROI output file: {0}\n\nIf there are no rows "
"in the output file, double-check your ROI/mask selection."
"\n\n".format(str(timeseries_file))
)

for line in roi_file_lines:
if "Mean_" in line:
try:
roi_list = line.split(",")
# clear out any blank strings/non ROI labels in the list
roi_list = [x for x in roi_list if "Mean" in x]
# rename labels
roi_list = [
x.replace("Mean", "ROI").replace(" ", "").replace("#", "")
for x in roi_list
]
except:
raise Exception(roi_err)
break
else:
raise Exception(roi_err)

if prefix:
temp_rois = []
for roi in roi_list:
roi = prefix + "_" + str(roi)
temp_rois.append(roi)
roi_list = temp_rois

return roi_list
Copy link
Member Author

Choose a reason for hiding this comment

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

This function isn't used anywhere except a commented-out section so I deleted both this function and the commented-out code.

CPAC/nuisance/utils/utils.py Show resolved Hide resolved
@shnizzedy shnizzedy force-pushed the fix/ruff-tests branch 4 times, most recently from 2597c1f to ecf36f3 Compare February 3, 2024 05:00
shnizzedy and others added 22 commits February 12, 2024 22:08
(some were `nibabel as nb`)
Where reformatting changed things like
> ("line 1"
>  " line 2")

to like

> "line 1" " line 2"

, change to like

> "line 1 line 2"
🎨 `print` statements → `log` statements
@sgiavasis sgiavasis merged commit 7d734c1 into initial-run/ruff Mar 5, 2024
43 checks passed
@shnizzedy shnizzedy deleted the fix/ruff-tests branch March 11, 2024 19:32
@shnizzedy shnizzedy added testing and removed tests labels Apr 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
python Pull requests that update Python code style testing
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

10 participants