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

Set global configuration for maxiters on all tests #244

Open
jeremy-myers opened this issue Sep 18, 2023 · 7 comments
Open

Set global configuration for maxiters on all tests #244

jeremy-myers opened this issue Sep 18, 2023 · 7 comments
Assignees
Labels
testing Testing related

Comments

@jeremy-myers
Copy link
Collaborator

Currently, explicitly defining maxiters= for various tests is done only on a partial basis. The motivation for limiting maxitersis that it will save CPU cycles and, ultimately, we do not care about the solution for the sake of the test. Some conversation can be found here: #241 (comment). One initial idea of @dmdunla is to set this to maxiters=10.

@jeremy-myers jeremy-myers added the testing Testing related label Sep 19, 2023
@dmdunla
Copy link
Collaborator

dmdunla commented Sep 19, 2023

@jeremy-myers Is there a mechanism that you have in mind that will support "global configuration"? Or is the intent to explicitly set maxiters to some specific value across all tests? I think the latter would be suitable, but I wanted to know if there is something that you know of that would support the former.

@jeremy-myers
Copy link
Collaborator Author

@jeremy-myers Is there a mechanism that you have in mind that will support "global configuration"? Or is the intent to explicitly set maxiters to some specific value across all tests? I think the latter would be suitable, but I wanted to know if there is something that you know of that would support the former.

I think exactly what you said, "explicitly set maxiters to some specific value across all tests", is what I had in mind. It looks pretty straightforward to set this in conftest.py. Let me know if this doesn't seem like the way to go.

@dmdunla
Copy link
Collaborator

dmdunla commented Sep 19, 2023

@jeremy-myers Is there a mechanism that you have in mind that will support "global configuration"? Or is the intent to explicitly set maxiters to some specific value across all tests? I think the latter would be suitable, but I wanted to know if there is something that you know of that would support the former.

I think exactly what you said, "explicitly set maxiters to some specific value across all tests", is what I had in mind. It looks pretty straightforward to set this in conftest.py. Let me know if this doesn't seem like the way to go.

I meant in the calls to the methods you would specify maxiters=10 -- e.g. , cp_apr(X, 2, maxiters=10). I'm worried that we would set something like this in conftest.py and forget it. If a problem arises in a test in the future, someone may not realize that the maximum number of iterations is set in some other place.

@jeremy-myers
Copy link
Collaborator Author

@jeremy-myers Is there a mechanism that you have in mind that will support "global configuration"? Or is the intent to explicitly set maxiters to some specific value across all tests? I think the latter would be suitable, but I wanted to know if there is something that you know of that would support the former.

I think exactly what you said, "explicitly set maxiters to some specific value across all tests", is what I had in mind. It looks pretty straightforward to set this in conftest.py. Let me know if this doesn't seem like the way to go.

I meant in the calls to the methods you would specify maxiters=10 -- e.g. , cp_apr(X, 2, maxiters=10). I'm worried that we would set something like this in conftest.py and forget it. If a problem arises in a test in the future, someone may not realize that the maximum number of iterations is set in some other place.

That's a fair point and will improve self-documentation of our tests. I'll reference this comment in the issue that I opened and take this task upon myself.

@jeremy-myers
Copy link
Collaborator Author

Decision

Explicitly set maxiters=10 in tests (vs. setting parameters in conftest.py where they may be forgotten in the future).

@jeremy-myers jeremy-myers self-assigned this Sep 19, 2023
@ntjohnson1
Copy link
Collaborator

Not to derail the conversation but if you make a maxiters fixture (or some other reasonable name) in conftest.py then it is pretty hard to forget about since it gets implicitly imported into the tests.

Usage would looks like:

def test_<some_name>(maxiters):
   algorithm(maxiters=maxiters)

Not a blocker but might be nicer than hardcoding it everywhere. There are a variety of test cleanups that would be nice to have but lower priority than the 2.0 stuff. Centralizing our sample tensors somewhere for easier re-use is one of those.

@jeremy-myers
Copy link
Collaborator Author

I'll let @dmdunla make the final call.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
testing Testing related
Projects
None yet
Development

No branches or pull requests

3 participants