-
Notifications
You must be signed in to change notification settings - Fork 221
Fix soft vs hard merge issue in analyzer #4025
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
Conversation
else: | ||
sparsity = None | ||
break |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To make sure people read carefully this is my main question? is this what a "hard" merge should do for sparsity?
@yger can you help on this ? |
@@ -960,20 +960,22 @@ def _save_or_select_or_merge( | |||
mergeable, masks = self.are_units_mergeable( | |||
merge_unit_groups, | |||
sparsity_overlap=sparsity_overlap, | |||
merging_mode=merging_mode, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@zm711 this should fix it. The implementation of this function is as follows:
- if "soft": return intersection of sparsity and check mergeability
- if "hard": return union and set mergeable to True
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wasn’t sure if in hard mode we wanted to test to see if soft was possible and switch to soft if possible and then only do hard if required.
@zm711 can you test if my last ocmmit fixed it? :) |
As it is a holiday here the person who discovered this in lab won’t be around. I can test on Monday with the dataset that triggered this. |
Oh right! Happy Independence Day and stop looking at the screen! Otherwise @samuelgarcia will need to intervene |
Tested and worked. |
Currently the code doesn't actually care if you are doing a soft or hard merge, it verifies the sparsity overlap for all units whether soft or hard and then errors even on hard merge if there is not an overlap. Confirmed by a labmate who ran
and got an error

To be honest I don't understand the merging mechanism well enough to know just setting sparsity to None for a hard merge is desired so I'm submitting this as a reminder that we should probably fix this before the next version. Sorry I don't really know how to make a MRE for this.
so help writing a test would be great.