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

Attempt to push the code for code review #1

Open
wants to merge 11 commits into
base: main
Choose a base branch
from
Open

Conversation

adatteq
Copy link
Collaborator

@adatteq adatteq commented Jul 2, 2021

No description provided.

src/alg_naive.py Outdated
@@ -0,0 +1,39 @@
import tools as t

def sp(n, k, L_t, d, unit_indices, results):
Copy link
Contributor

Choose a reason for hiding this comment

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

Naming: I'd give the function a longer and hence more descriptive name (e.g. shapley_value or compute_sv or something like that). Replace the single-letter names with more descriptive ones as well (e.g. n with num_units). All names should be small caps and underscores (e.g. L_t violates this, apart from violating the previous point as well).

units are turned on at the start, and in the second case, 3 need to be on at the
start, so the computation of the DP table is different.
'''
def sp(n_unit, k, L_t, Dp, d, uI, res):
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as in the other file: function name, variable names, usage of capital letters...


# Define the range for N we want to test and the number of repetition
# for each pair (N, K)
n_lower_bound = 2
Copy link
Contributor

Choose a reason for hiding this comment

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

All constants should be in ALL_CAPS.

N = n

# K from KNN classifier
K = k
Copy link
Contributor

Choose a reason for hiding this comment

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

These assignments seem uneccessary. Also K on the left is a variable I guess, so it should be named with small caps.

src/tools.py Outdated
return summ

'Return a copy of the target vector'
def copy_vector(target):
Copy link
Contributor

Choose a reason for hiding this comment

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

This can be done with the copy built-in library.

src/alg_naive.py Outdated
# Compute the marginal contribution from the soft accuracy function
diff = prop_with_q-prop_without_q
vMv = diff/k
count = t.count_ones(vector)
Copy link
Contributor

Choose a reason for hiding this comment

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

Could be a simple single line, for example length([x for x in vector if x == 1]). In those cases it is excessive to put it in a function. Alternatively, you could use numpy to do these vector operations. It might be faster as numpy is highly optimized.

src/tools.py Outdated
tempScp.append(count(Di, D_c, 1, L_t))
return [tempTb, tempSc, tempTbp, tempScp]

def minus(v):
Copy link
Contributor

Choose a reason for hiding this comment

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

These minus and sum functions could surely be replaced with numpy vector operations.

src/tools.py Outdated
return j

'Compute n choose k'
def choose(n, k):
Copy link
Contributor

Choose a reason for hiding this comment

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

There's a single function in numpy.

src/tools.py Outdated
array :
array of tuple of the form [distance from D_t, label, id]
'''
def sort(array):
Copy link
Contributor

Choose a reason for hiding this comment

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

This can be solved using the sorted built-in function with defining a custom key, see here.

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.

None yet

2 participants