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

Fix relative cav #111

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

Fix relative cav #111

wants to merge 13 commits into from

Conversation

monz
Copy link
Contributor

@monz monz commented May 8, 2021

  • the paper states in section 3.6 for relative tcav, construct a positive and negative set, e.g., for concepts P_dot, P_stripe, P_mesh this would look like, P_stripe (positive) and {P_dot U p_mesh} (negative). However, the code was giving sets like, P_stripe (pos), P_dot (neg). Now, the complement set is used for relative tcav as described in the paper.
  • when computing relative tcav, plot function failed because concepts are also 'random' concepts and is_random_concept(concept) is always true, therefore. Now, plot for relative tcav handles 'random' concepts correctly.

Hope I did understand the paper correctly and gave a proper fix.

monz added 5 commits May 8, 2021 20:31
- the paper states in section 3.6 for relative tcav, construct a positive
  and negative set, e.g., for concepts P_dot, P_stripe, P_mesh this would
  look like, P_stripe (positive) and {P_dot U p_mesh} (negative).
  However, the code was giving sets like, P_stripe (pos), P_dot (neg).
  Now, the complement set is used for relative tcav as described in the
  paper.
- when computing relative tcav, plot function failed because concepts are
  also 'random' concepts and is_random_concept(concept) is always true,
  therefore. Now, plot for relative tcav handles 'random' concepts
  correctly.
@BeenKim
Copy link
Contributor

BeenKim commented May 10, 2021

Thank you for submitting this RP @monz ! Two things if you are able 1) mind changing the relative_cav variable to be is_relative_cav (to avoid confusion that the variable IS the cav) 2) this is big enough change that I feel a few tests could be very helpful. Would you mind adding a few simple ones to our test deck? Thank you!
been

@monz
Copy link
Contributor Author

monz commented May 15, 2021

Hi @BeenKim, please have a look to the added tests, hope this is what you expected, kind regards, Markus.

@monz
Copy link
Contributor Author

monz commented Jun 11, 2021

Hi @BeenKim I know its kind of a big PR, however, I am curious what you think about it. Kind Regards, Markus.

@BeenKim
Copy link
Contributor

BeenKim commented Jun 11, 2021 via email

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