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

Multiple parallel optimizations #77

Merged
merged 34 commits into from
Nov 6, 2019
Merged

Multiple parallel optimizations #77

merged 34 commits into from
Nov 6, 2019

Conversation

ChristianZimpelmann
Copy link
Member

Run several optimizations in parallel (args, criterium etc. are then entered as a list)

First: all relevant arguments need to be entered as a list.

@ChristianZimpelmann
Copy link
Member Author

The code could be a bit shorter if also single optimizations are broadcasted (to lists of length 1).

Then this part is not needed:

if n_opts == 1:
        # Run just a single optimization
        result = _single_minimize(
            criterion=criterion,
            params=params,
            algorithm=algorithm,
            criterion_args=criterion_args,
            criterion_kwargs=criterion_kwargs,
            constraints=constraints,
            general_options=general_options,
            algo_options=algo_options,
            dashboard=dashboard,
            db_options=db_options,
        )

However, I think it is clearer to make this distinction and don't initialize multiprocessing if it is unnecessary.

@janosg
Copy link
Member

janosg commented Sep 30, 2019

Yes, we should keep single optimization as special case!

@janosg janosg mentioned this pull request Oct 9, 2019
estimagic/optimization/optimize.py Outdated Show resolved Hide resolved
estimagic/optimization/optimize.py Outdated Show resolved Hide resolved
estimagic/optimization/process_arguments.py Outdated Show resolved Hide resolved
estimagic/optimization/process_arguments.py Outdated Show resolved Hide resolved
estimagic/optimization/process_arguments.py Outdated Show resolved Hide resolved
estimagic/optimization/process_arguments.py Outdated Show resolved Hide resolved
tox.ini Outdated Show resolved Hide resolved
Copy link
Member

@janosg janosg left a comment

Choose a reason for hiding this comment

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

The changes made the code much more readable. Just two questions left, then we can merge. Thanks!

estimagic/optimization/optimize.py Outdated Show resolved Hide resolved
estimagic/optimization/process_arguments.py Show resolved Hide resolved
@ChristianZimpelmann
Copy link
Member Author

The changes made the code much more readable. Just two questions left, then we can merge. Thanks!

I took care of your remarks and simplified the code once more (the broadcasting function isn't needed anymore)

Also n_opts is now just len(arguments). So I suggest to not return it separately.

Copy link
Member

@janosg janosg left a comment

Choose a reason for hiding this comment

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

Very nice. Thanks a lot for this PR!

@janosg janosg merged commit 1cdbec7 into master Nov 6, 2019
@janosg janosg deleted the multiple_opt branch November 6, 2019 15:00
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.

None yet

2 participants