Conversation
|
@dpo I rebased on the your changes and cleaned up the git pushes |
| CUTEst problems). | ||
|
|
||
| #### Keyword arguments | ||
| * `use_threads::Bool`: whether to use threads (default: `true`); |
There was a problem hiding this comment.
Why do we need this keyword? Threads should be enabled/disabled with the environment variable.
There was a problem hiding this comment.
I think the use_threads keyword is valuable to keep, even with the environment variable, for the following reasons:
Testing and Debugging: This flag allows us to explicitly override the environment variable during testing or debugging. It’s particularly useful to verify that the application behaves correctly in both threaded and non-threaded modes without re-writing the functionality in the unit tests.
Fine-Grained User Control: Some users may want to override the global threading setting for specific scenarios:
They might want to disable threading for this application while leaving it enabled for other programs to avoid resource contention.
I think keeping this flag ensures flexibility and adaptability, making the tool more useful for both development and usage scenarios.
There was a problem hiding this comment.
A user who doesn’t set JULIA_NUM_THREADS is not expected anything to run multi-threaded, yet use_threads will be true. I think that only introduces confusion.
| kwargs..., | ||
| ) where {TName} | ||
|
|
||
| # Collect information about counters |
There was a problem hiding this comment.
Please remove these comments and blank lines that do not have anything to do with the new functionality.
|
|
||
| for (id, problem) in enumerate(problems) | ||
| # Convert problems to an indexable vector | ||
| problem_list = collect(problems) |
There was a problem hiding this comment.
We really do not want this. If problems is a generator of all CUTEst models, this line will try to materialize them all at once and hold them all in memory simultaneously, which is not acceptable.
|
Hi @farhadrclass, i am very interested in this functionality. |
|
@MaxenceGollier, thanks for finding the issue, |
3 participants
Cleaned up the code and rebased on newest changes from #167