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

8185 test refactor 2 #8405

Open
wants to merge 12 commits into
base: dev
Choose a base branch
from

Conversation

garciadias
Copy link
Contributor

@garciadias garciadias commented Mar 28, 2025

Fixes #8185

Description

This PR solves items 2 and 3 on #8185 for a few test folders.
I would merge these and proceed with the same type of change in other files if @ericspod approves.

I would like to keep these PRs small, so even if they have the same pattern of changes, merging them bit by bit would make them more manageable.

A few sentences describing the changes proposed in this pull request.

Types of changes

  • Non-breaking change (fix or new feature that would not break existing functionality).
  • Breaking change (fix or new feature that would cause existing functionality to change).
  • New tests added to cover the changes.
  • Integration tests passed locally by running ./runtests.sh -f -u --net --coverage.
  • Quick tests passed locally by running ./runtests.sh --quick --unittests --disttests.
  • In-line docstrings updated.
  • Documentation updated, tested make html command in the docs/ folder.

@garciadias
Copy link
Contributor Author

Hi @ericspod,

Could you please review this PR?
I think it is all good to follow your specification on the #8185 issue.
I am sending the modification to these few tests so we can limit the size of the PR.
If you approve, I will proceed with more of these simplifications.

Comment on lines +81 to +89
boxes_mask_1 = [[[-1, 0], [0, -1]]]
for params in dict_product(ndarray_type=TEST_NDARRAYS):
p = params["ndarray_type"]
TESTS_2D_mask.append([p(boxes_mask_1), (p([[0.0, 0.0, 2.0, 2.0]]), p([0]))])

boxes_mask_2 = [[[-1, 0], [0, -1]], [[-1, 1], [1, -1]]]
for params in dict_product(ndarray_type=TEST_NDARRAYS):
p = params["ndarray_type"]
TESTS_2D_mask.append([p(boxes_mask_2), (p([[0.0, 0.0, 2.0, 2.0], [0.0, 0.0, 2.0, 2.0]]), p([0, 1]))])
Copy link
Member

Choose a reason for hiding this comment

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

If there's only one key passed to dict_product, this isn't saving any space or complexity so I wouldn't use this here.

Comment on lines +41 to +42
for params in dict_product(device=TEST_DEVICES, dtype=DTYPES):
TESTS.append((*params["device"], *params["dtype"])) # type: ignore
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
for params in dict_product(device=TEST_DEVICES, dtype=DTYPES):
TESTS.append((*params["device"], *params["dtype"])) # type: ignore
TESTS = [(*p["device"], *p["dtype"]) for p in dict_product(device=TEST_DEVICES, dtype=DTYPES)] # type: ignore

In some places a list comprehension may be fine to use, and not have to have TESTS = [] before.

Comment on lines +29 to +55
for params in dict_product(
dropout_rate=[0.5],
in_channels=[1, 4],
hidden_size=[96, 288],
img_size=[32, 64],
patch_size=[8, 16],
num_heads=[8, 12],
proj_type=["conv", "perceptron"],
pos_embed_type=["none", "learnable", "sincos"],
nd=[2, 3],
):
test_case = [
{
"in_channels": params["in_channels"],
"img_size": (params["img_size"],) * params["nd"],
"patch_size": (params["patch_size"],) * params["nd"],
"hidden_size": params["hidden_size"],
"num_heads": params["num_heads"],
"proj_type": params["proj_type"],
"pos_embed_type": params["pos_embed_type"],
"dropout_rate": params["dropout_rate"],
"spatial_dims": params["nd"],
},
(2, params["in_channels"], *[params["img_size"]] * params["nd"]),
(2, (params["img_size"] // params["patch_size"]) ** params["nd"], params["hidden_size"]),
]
TEST_CASE_PATCHEMBEDDINGBLOCK.append(test_case)
Copy link
Member

@ericspod ericspod Apr 4, 2025

Choose a reason for hiding this comment

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

Suggested change
for params in dict_product(
dropout_rate=[0.5],
in_channels=[1, 4],
hidden_size=[96, 288],
img_size=[32, 64],
patch_size=[8, 16],
num_heads=[8, 12],
proj_type=["conv", "perceptron"],
pos_embed_type=["none", "learnable", "sincos"],
nd=[2, 3],
):
test_case = [
{
"in_channels": params["in_channels"],
"img_size": (params["img_size"],) * params["nd"],
"patch_size": (params["patch_size"],) * params["nd"],
"hidden_size": params["hidden_size"],
"num_heads": params["num_heads"],
"proj_type": params["proj_type"],
"pos_embed_type": params["pos_embed_type"],
"dropout_rate": params["dropout_rate"],
"spatial_dims": params["nd"],
},
(2, params["in_channels"], *[params["img_size"]] * params["nd"]),
(2, (params["img_size"] // params["patch_size"]) ** params["nd"], params["hidden_size"]),
]
TEST_CASE_PATCHEMBEDDINGBLOCK.append(test_case)
for params in dict_product(
dropout_rate=[0.5],
in_channels=[1, 4],
hidden_size=[96, 288],
img_size=[32, 64],
patch_size=[8, 16],
num_heads=[8, 12],
proj_type=["conv", "perceptron"],
pos_embed_type=["none", "learnable", "sincos"],
spatial_dims=[2, 3],
):
nd = params["spatial_dims"]
args = {**params, "img_size": (params["img_size"],) * nd, "patch_size": (params["patch_size"],) * nd}
input_shape = (2, params["in_channels"], *[params["img_size"]] * nd),
expected_shape = (2, (params["img_size"] // params["patch_size"]) ** nd, params["hidden_size"]),
TEST_CASE_PATCHEMBEDDINGBLOCK.append([args, input_shape, expected_shape])

With some changes to argument choices you can eliminate needing to make an argument dictionary by using the initial dictionary from dict_product. Elsewhere you can do the same buy may need to remove keys from args where they aren't desired. Same idea for the other changes in this file.

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.

Test Refactor
2 participants