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

[Diversity module] Add Formulas to Docstrings, Add Tests, & Polish Module #130

Open
2 of 3 tasks
FarnazH opened this issue Jun 9, 2023 · 2 comments
Open
2 of 3 tasks
Assignees

Comments

@FarnazH
Copy link
Member

FarnazH commented Jun 9, 2023

See: #121

For each function (mentioned in #121), make sure that:

  • implementation is clear with comments.
  • docstring is polished and gives the formula implemented (add a reference to the paper used for implementation)
  • tests have 100% coverage
@AWBroscius
Copy link
Collaborator

While trying to write documentation for this module, these are some of the implementation errors I ran into:

  • the logdet function, as currently implemented according to this source, requires a features x mols matrix, while elsewhere in the package our standard is a mols x features matrix. Do I change the implementation or just note this in the docs?

  • shannon_entropy() is not correctly implemented, and does not match the cited paper/equation. There is a comment to add a raise error, but reading through the cited paper I see no reason why there would be a value error in its calculation. Do you want me to rewrite this function?

  • total_diversity_volume is also not implemented according to its paper. The function currently returns the degree of overlap between molecule neighborhoods, not the total diversity volume. I think we should rename the function instead of altering the implementation, as in the paper, total diversity volume does not describe a sub-set of data points, but the entire input space.

  • The Wasserstien distance function wdud() needs to be rewritten

@FarnazH
Copy link
Member Author

FarnazH commented Jun 20, 2023

Post - PR #135:

@FanwangM FanwangM self-assigned this Jun 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants