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

#484: Keep tempered algorithm in sync with implementation in vt #485

Merged
merged 11 commits into from
Dec 18, 2023
Merged

#484: Keep tempered algorithm in sync with implementation in vt #485

merged 11 commits into from
Dec 18, 2023

Conversation

ppebay
Copy link
Contributor

@ppebay ppebay commented Dec 14, 2023

resolves #484

@ppebay ppebay changed the title Keep tempered algorithm in sync with implementation in vt #484: Keep tempered algorithm in sync with implementation in vt Dec 14, 2023
@ppebay ppebay requested a review from cwschilly December 17, 2023 21:30
@ppebay ppebay marked this pull request as ready for review December 17, 2023 21:30
@ppebay ppebay self-assigned this Dec 17, 2023
@cwschilly cwschilly force-pushed the 484-Keep-tempered-algorithm-in-sync-with-implementation-in-vt branch from 3ed372d to 330114e Compare December 18, 2023 15:14
@cwschilly
Copy link
Contributor

@ppebay This PR now passes all tests

cwschilly
cwschilly reviewed Dec 18, 2023
View reviewed changes
Copy link
Contributor

@cwschilly cwschilly left a comment

Choose a reason for hiding this comment

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

I can write a unit test for the max_clusters parameter if it would be helpful--just let me know

src/lbaf/IO/lbsConfigurationValidator.py Show resolved Hide resolved
src/lbaf/Execution/lbsTransferStrategyBase.py Outdated Show resolved Hide resolved
@cwschilly cwschilly self-requested a review December 18, 2023 17:37
cwschilly
cwschilly approved these changes Dec 18, 2023
View reviewed changes
Copy link
Contributor

@cwschilly cwschilly left a comment

Choose a reason for hiding this comment

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

LGTM

@ppebay
Copy link
Contributor Author

ppebay commented Dec 18, 2023

@cwschilly When undefined, the new parameter is consider to be infinite (before that addition, there was a hard-coded value of 10 in the code). When the value is not provided, it's going to be considered infinite -- which in turn might cause the code to go into a computationally intractable cluster swap exploration.

As a result other CI tests might have come unfeasible unless the parameter is explicitly added to their configuration file.

@ppebay ppebay requested a review from cwschilly December 18, 2023 17:53
cwschilly
cwschilly approved these changes Dec 18, 2023
View reviewed changes
Copy link
Contributor

@cwschilly cwschilly left a comment

Choose a reason for hiding this comment

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

The CI is hanging occasionally, but I have not been able to find anything in this PR that causes it. Any hung jobs will pass on a rerun.

@ppebay ppebay merged commit c493621 into develop Dec 18, 2023
8 checks passed
@ppebay ppebay deleted the 484-Keep-tempered-algorithm-in-sync-with-implementation-in-vt branch December 18, 2023 20:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cwschilly cwschilly cwschilly approved these changes

Assignees

@ppebay ppebay

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Keep tempered algorithm in sync with implementation in vt
2 participants
@ppebay @cwschilly