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

peer review #39

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

peer review #39

wants to merge 5 commits into from

Conversation

yvanlaere
Copy link

Hi Triana
I liked your notebook, it's clear and concise. Below are a few comments and suggestions.

Maybe you could explain what would change for an unbalanced assignment if they can be solved with the hungarian algorithm. If they can't be solved with the hungarian algorithm, maybe you could mention what algorithm can solve it.

In this sentence in the second step of the outline of the approach:

Mark the rows and columns which have 0 entries

It may not be entirely clear that the entries are zeros instead of there being no entries.

Some typo's and other minor details:

in the introduction:

When there are the same number of agents than tasks, then the problem is called balanced assignment, otherwise it is called unbalanced assignment.

in the introduction:

It can be represented as an adjacency matrix,

step 3 of the outline of the approach:

then the optimal number of zeroes is

description of find_min_row:

readjusts

in the Hungarian_algorithm function:

'mat`: the cost matrix

One of the apostrophes is the wrong kind

In the explanation of the Hungarian algorithm it says:

This function performs steps 1 to 4, by calling the previous functions.

These functions are now explained below the hungarian_algorithm function

in find_min_row: maybe change the indentation here:

for row in 1:size(matrix_zero)[1]
#if true in matrix_zero[row, :] # Needs a 0 in the row
# If the number of zeros < than the last min stored
if sum(matrix_zero[row,:]) > 0 && min_row[1] > sum(matrix_zero[row,:])
#stores the number of zeros and the row index
min_row = [sum(matrix_zero[row,:]), row]
end
end

Yari Van Laere

@milanghe
Copy link

Hi, i had an exam today so I couldn't do the review in the morning, but I can't seem to open a separate pull request anymore so I will add my review as a comment.

I agree that it's short and simple, which is very good. The essentials are explained and demonstrated with a clear example. Only remark is that there are still a few spelling errors and that the helper functions are shown after the main hungarian_algorithm method.

good job

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.

3 participants