Open schachem opened 1 year ago
for i in range(n_inst):
e = 0
for j in range(n_attr - 1): # IS THIS CORRECT?
j_value = X_view[i, j]
if j_value != (max_bin - 1):
for k in range(j, n_attr):
k_value = X_view[i, k]
if k_value != (max_bin - 1):
e -= (coupling_view[j * (max_bin - 1)
+ j_value, k * (max_bin - 1) + k_value])
e -= (fields_view[j * (max_bin - 1) + j_value])
energies_view[i] = e
return energies
In my endeavours to somehow extract the local fields and couplings that were used to calculate a specific energy value I stumbled upon the compute_energy function that uses a for loop as depicted above.
I will not pretend that I fully understood the entire math from the paper, but to me it looks like that the for loop always discards the last attribute? Is this really wanted behavior or a bug?
I would be very thankful for a response or more insights as I really like the papers approach and white-box characteristics that would in theory allow to really make the classifiers decisions explainable :)
Best Regards Markus
After further playing around, I come to the conclusion that it should be for j in range(n_attr )
and for k in range(j, n_attr-1)
I come to that conclusion rather empirically, as with some print statements added this actually yields the correct number of field values and couplings for my setup, i.e. 9 attributes.
With the "-1" in the ranges switched as described this correctly leads to 9 field values and 36 coupling values that are summed up together for the energy value.
It would be great if you could confirm my assumptions.
Thanks, Markus
Hi @schachem . Thank you for your observations! First, about being able to see the coupling values and local fields of an instance, this functionality was really missing. I just added a method for this in EFC and an example of usage (breaking_energy.py). I hope it helps!
Now, about the second question, there is indeed an error with these intervals. But the correct version would be
for i in range(n_inst):
e = 0
for j in range(n_attr):
j_value = X_view[i, j]
if j_value != (max_bin - 1):
for k in range(j+1, n_attr):
k_value = X_view[i, k]
if k_value != (max_bin - 1):
e -= (coupling_view[j * (max_bin - 1)
+ j_value, k * (max_bin - 1) + k_value])
e -= (fields_view[j * (max_bin - 1) + j_value])
energies_view[i] = e
return energies
So that j iterates over all attributes, and k over all attributes strictly greater than j.
The way you suggested, we would not be adding the pairwise couplings for the last attribute and we would be counting the couplings of a feature with itself, which are disposable. You would end up with the same amount of values in the sum, but they would be wrong. I have already updated the repository with this fix.
Thanks again for your contribution, @schachem ! Any other questions, feel free to send a message.
Hey @munak98,
first of all, thank you very much for getting back to me!
I have two further questions:
1) The Fix you applied in your latest commit would also have to be applied to the newly added _breakdown_energy function I guess, right?
2) Regarding the fix itself and the discussion about ranges:
I also came to the same conclusion as you from a logical perspective. However, after having tried that the estimators estimations did go completely wrong. You can also try the binary_example that is included in the repository now after your fix. It basically does not work anymore.
Binary Example before your fix:
Binary Example after your fix:
Similar behavior was occuring with my own data. So I do not pretending to understand what is going on here.
I can only say that only looking at the loop and reasoning about the ranges, I would agree with you regarding the fix. Looking at the results, I would say that can not be right, and the way it was before was maybe not so wrong in the end? Even with my proposed "fix", i.e. setting the ranges as in my first comment (although I agree with your comment why that should not make sense), leads to working predictions.
I greatly appreciate your time and hope we can get to the bottom of this! But right now after the fix, it looks to me like the estimator is not working like it should.
Greetings, Markus
Yes! My mistake, sorry.
About the intervals, actually the correct version would be with for j in range(n_attr)
and for k in range(j, n_attr)
. This way we include the last attribute and keep the couplings of a feature with itself, which are actually not disposable.
I updated the repo with this change!
Good morning @munak98,
thank you again for your efforts.
Thank you for clarifying that couplings of features with themselves are actually not disposable.
My understanding from reading the paper was that we only look at couplings of different features. Nevertheless, now it seems to work as it should.
I have opened a small PR regarding an indention error in the breakdown energy function. I guess after that, all questions and issues are resolved :)
Greetings from Munich, Markus
If I understood correctly, the beauty of the EFC is that we can see which features of a sample did contribute the most toward a elevated total energy (potentially malicious).
However, the estimator only returns the total energies of classified samples and not the couplings themselves (that are added up I think)?
Is their a way to see the couplings of a sample?