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

Update OverlapCalculator.py #214

Closed
wants to merge 1 commit into from
Closed

Update OverlapCalculator.py #214

wants to merge 1 commit into from

Conversation

ptrab
Copy link

@ptrab ptrab commented Jun 2, 2022

adding the functionality to use the Kuhn-Munkres algorithm to update the tracked root.
can be activated through the calc-argument min_cost: True and default is False currently.


As an example, let's assume we are tracking excited state number 8 (old 8) and in row 7 and 8 and also column 7 and 8 of an overlap matrix between two optimization steps, there are the following values:

       new 7 | new 8
old 7 | 0.65 | 0.71
old 8 | 0.54 | 0.56

For old 8 we see that new 7 and new 8 are very similar and, based on the current algorithm, we would choose new 8 because of its higher overlap (56% vs. 54%) in this row and, therefore, not update the root. However, if we take a look at the column for new 8, we see that old 7 and new 8 have a much higher overlap of 71% (vs. 56%). As such, new 8 is more likely to be old 7 than old 8 and vice versa.

Using the Kuhn-Munkres algorithm, each old excited state gets mapped to only one new excited state based on the whole overlap matrix which should be more reasonable than taking only a single row into consideration while ignoring all other information.


I did not outsource the new code into a new function ... sorry :D

adding the functionality to use the Kuhn-Munkres algorithm to update the tracked root. can be activated through the calc-argument "min_cost: True" and default is False currently
Copy link
Owner

@eljost eljost left a comment

Choose a reason for hiding this comment

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

Please capitalize the beginnings of the comments.

# to make the assignment more reasonable and omit potential
# double assignments from rows being treated isolated
self.log(
"Analyzing full overlap matrix and find most reasonable"
Copy link
Owner

Choose a reason for hiding this comment

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

add one space after reasonable

"another row mapping to the same state had a higher "
"overlap."
)
max_overlap = ref_root_row[new_root]
Copy link
Owner

Choose a reason for hiding this comment

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

Delete this line and move (the same) line 803 outside the else block.

@ptrab
Copy link
Author

ptrab commented Jun 2, 2022

Currently it is also highly confusing in these few lines of code as col_ind and row_ind are very similar to col_inds and row_inds but should not be confused. The "singular" forms are the ones from the current code, while the "plural" forms hold the results from the new linear_sum_assignment.

@eljost eljost force-pushed the master branch 2 times, most recently from 92f2f54 to dc99913 Compare October 7, 2022 06:41
@eljost eljost closed this Dec 20, 2023
eljost pushed a commit that referenced this pull request Jan 5, 2024
as originally described in #214.
fix: pysisplot -o was broken
eljost pushed a commit that referenced this pull request Oct 2, 2024
as originally described in #214.
fix: pysisplot -o was broken
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