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

Test Refactor #8185

Open
ericspod opened this issue Oct 31, 2024 · 4 comments · May be fixed by #8231
Open

Test Refactor #8185

ericspod opened this issue Oct 31, 2024 · 4 comments · May be fixed by #8231
Assignees
@ericspod @Nic-Ma @KumoLiu
Labels
Contribution wanted enhancement New feature or request Low risk nice-to-have features refactor Non-breaking feature enhancements

Comments

@ericspod
Copy link
Member

ericspod commented Oct 31, 2024 •
edited
Loading

The tests directory has gotten large over time as we've added new components with their test cases. It's absolutely a good thing to have thorough testing but as a almost-flat directory it's hard to find things and it's cumbersome to view so many files in IDEs. Further there are likely many areas of refactoring that we can do to reduce duplicate code and introduce more helper routines to do common tasks. I would suggest a few things to improve our tests:

  • Break the contents of tests into subdirectories mirroring those in the monai directory. Tests for transforms would go under transforms, those for networks under networks, etc. It may be necessary to have more directory structure under these as well but this doesn't need to be overcomplicated.
  • Common patterns and ways of generating test data can be factored out into utils.
  • A common pattern is to generate test cases for use with parameterized in deeply-nested for loops, eg.:
TEST_CASES=[]
for arg1 in [2,4]:
    for arg2 in [8,16]:
        TEST_CASES.append([{"arg1": arg1, "arg2": arg2}, arg1, arg2])

A simple routine for doing product over dictionaries can reduce this code significantly:

def dict_product(**items):  # should be put in core utils somewhere
    keys=items.keys()
    values=items.values()
    for pvalues in product(*values):
        yield dict(zip(keys, pvalues))
...
TEST_CASES=[[d, d["arg1"], d["arg2"]] for d in dict_product(arg1=[2,4], arg2=[8,16])]
  • A number of tests use large data items or perform quite a few iterations of training. These are slow running so it would be good to go through the slower tests to see if any speedup can be done without losing testing value.
  • Similarly how synthetic data is created should be re-evaluated to see what commonality can be factored out and standardised.
  • Many tests are silent while others have output whether they pass or not. My feeling was always that tests should be silent unless they fail then they should be loud about why. This keeps the clutter down so it's clear when tests are passing, and when they don't their output is easier to spot and understand. This is maybe personal taste but should we rethink this behaviour?
@ericspod ericspod added enhancement New feature or request Low risk nice-to-have features refactor Non-breaking feature enhancements labels Oct 31, 2024
@garciadias
Copy link

Hi there. I am interested in contributing, and this issue is something I would be comfortable helping with. Following the contribution guide, I am communicating. 🙃
Should I open a pull request before starting or as soon I have a done a little work?

@ericspod
Copy link
Member Author

Hi @garciadias thanks for offering to help. Did you want to do one aspect of what I was proposing or more? I would suggest choosing which one you want to do first, consider how much work that would be, then starting implementing some things on your own. I would suggest waiting until you have something to review before raising the PR, unless you want help at some point in which case you can open a draft PR so we can look at your code. All these changes are important in my mind somewhat equally so I'll leave it to you to pick something to start with, let me know what you decide and we can discuss from there.

@garciadias
Copy link

garciadias commented Nov 19, 2024 •
edited
Loading

Hi @ericspod,

Thank you for your answer.

I was thinking about starting with this one:

  • A number of tests use large data items or perform quite a few iterations of training. These are slow running so it would be good to go through the slower tests to see if any speedup can be done without losing testing value.

I see the -q option skips the tests flagged as skip_if_quick, so my approach would be to look into the tests with this flag to understand which are actual integration tests and which could mock heavy functions. My main goal would be to increase the coverage with the -q option so you minimise the amount of slow tests that need to be run, making the development process more smooth.

Hopefully, this would give me enough knowledge of the test base so I could proceed with the first point:

  • Break the contents of tests into subdirectories mirroring those in the monai directory. Tests for transforms would go under transforms, those for networks under networks, etc. It may be necessary to have more directory structure under these as well but this doesn't need to be overcomplicated.

What do you think?

@ericspod
Copy link
Member Author

Hi @ericspod,

Thank you for your answer.

I was thinking about starting with this one:

  • A number of tests use large data items or perform quite a few iterations of training. These are slow running so it would be good to go through the slower tests to see if any speedup can be done without losing testing value.

I see the -q option skips the tests flagged as skip_if_quick, so my approach would be to look into the tests with this flag to understand which are actual integration tests and which could mock heavy functions. My main goal would be to increase the coverage with the -q option so you minimise the amount of slow tests that need to be run, making the development process more smooth.

Hopefully, this would give me enough knowledge of the test base so I could proceed with the first point:

  • Break the contents of tests into subdirectories mirroring those in the monai directory. Tests for transforms would go under transforms, those for networks under networks, etc. It may be necessary to have more directory structure under these as well but this doesn't need to be overcomplicated.

What do you think?

That sounds like a good plan so go for it. My feeling with the slow tests is that they might be using data that's much larger than needed for the same testing value, as well as running for too long. Testing things for functionality and accuracy are two different tasks and I wonder if there's too much focus in places on getting the right result. Definitely have a look around and see what you come up with, you can also make a new issue if you like detailing your findings if more discussion would help.

@ericspod ericspod moved this to In progress in MONAI v1.5 Nov 21, 2024
@garciadias garciadias linked a pull request Nov 21, 2024 that will close this issue
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@ericspod ericspod

@Nic-Ma Nic-Ma

@KumoLiu KumoLiu

Labels
Contribution wanted enhancement New feature or request Low risk nice-to-have features refactor Non-breaking feature enhancements
Projects
MONAI v1.5
Status: In progress
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

8185 - Refactor test garciadias/MONAI
4 participants
@garciadias @ericspod @Nic-Ma @KumoLiu