sandialabs / pyttb

Python Tensor Toolbox
https://pyttb.readthedocs.io
BSD 2-Clause "Simplified" License
26 stars 13 forks source link

Set global configuration for maxiters on all tests #244

Open jeremy-myers opened 1 year ago

jeremy-myers commented 1 year ago

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: https://github.com/sandialabs/pyttb/pull/241#discussion_r1327971938. One initial idea of @dmdunla is to set this to maxiters=10.

dmdunla commented 1 year ago

@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 commented 1 year ago

@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 commented 1 year ago

@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 commented 1 year ago

@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 commented 1 year ago

Decision

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

ntjohnson1 commented 1 year ago

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 commented 1 year ago

I'll let @dmdunla make the final call.