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

added PrioriKnowledge to ttpm #159

Merged
merged 1 commit into from
Oct 31, 2024
Merged

Conversation

juliusd01
Copy link
Contributor

As mentioned in #79 , it would be great, if more algorithms would be able to use PrioriKnowledge. I added this for the TTPM algorithm.

@kelizhang
Copy link
Member

Thank you for your effort in enhancing the usability of TTPM. We would be pleased to merge your work into our main branch after a few modifications to your current commit.

Please feel free to ignore some minor issues, as we’ll address them during the merge. However, here are a few points for you to consider or clarify:

  1. In the learn() function: To enhance robustness, just suggest you consider implementing it as follows,

    edge_mat[self.priori_knowledge.matrix == 1] = 1
  2. In the _one_step_change() function: Your logic is sound, but the following structure might improve clarity:

    j, i = e
    if j == i:
        return edge_mat
    
    new_edge_mat = edge_mat.copy()
    
    # Only delete the edge if removal is allowed
    if new_edge_mat[j, i] == 1 and not priori_knowledge.is_required(j, i):
        new_edge_mat[j, i] = 0
    
    # Only add the edge if addition is allowed
    if new_edge_mat[j, i] == 0 and not priori_knowledge.is_forbidden(j, i) and not priori_knowledge.is_required(i, j):
        new_edge_mat[j, i] = 1
        new_edge_mat[i, j] = 0
    
    return edge_mat

@kelizhang
Copy link
Member

As mentioned in #79 , it would be great, if more algorithms would be able to use PrioriKnowledge. I added this for the TTPM algorithm.

if you agree, we'll merge your commit first, and then tweak it.

@juliusd01
Copy link
Contributor Author

Yes, that's fine for me

@kelizhang kelizhang merged commit 9a5fefe into huawei-noah:master Oct 31, 2024
1 check passed
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