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

Fix auto dem condition flow #734

Closed
wants to merge 3 commits into from
Closed

Conversation

inmzhang
Copy link
Contributor

In my understanding, the condition flow in the sinter.WorkIn.auto_dem() should be:

(decompose_errors=True, flatten_loops=False) -> (decompose_errors=True, flatten_loops=True) ->(decompose_errors=False, flatten_loops=False) ->(decompose_errors=False, flatten_loops=True)

while the current code tried (decompose_errors=True, flatten_loops=False) and (decompose_errors=False, flatten_loops=False) twice, which seems like typos. Did I miss something?

@Strilanc
Copy link
Collaborator

Urgh that's embarrassing that I got the order wrong. But I think this PR still has the wrong order. It should be

{flatten,decompose}
{decompose}
{flatten}
{}

The reason for flatten+decompose first is because flattening makes the conversion faster and the dem's file size smaller. There's a bug where it can cause the decomposition to fail, so there's the fallback to no-flattening. Probably this should just be changed to a fallback to no-flattening yes-decompose with ignore_decomposition_failures=True.

I'll think about it some more.

@Strilanc Strilanc force-pushed the main branch 3 times, most recently from 7c26e1c to e5172a4 Compare September 10, 2024 06:24
Strilanc added a commit that referenced this pull request Sep 21, 2024
Derived from #734  , adjusted for major code updates since then

Co-Author: @inmzhang
@Strilanc
Copy link
Collaborator

Moved this change into #832

@Strilanc Strilanc closed this Sep 21, 2024
Strilanc added a commit that referenced this pull request Sep 24, 2024
Derived from #734 , adjusted for
major code updates since then

Co-Author: @inmzhang
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.

2 participants