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

Test notebook #147

Merged
merged 11 commits into from
Aug 5, 2023
Merged

Test notebook #147

merged 11 commits into from
Aug 5, 2023

Conversation

marco-2023
Copy link
Collaborator

Made necessary changes so that the notebook runs. Added some observations regarding some methods' results to the notebook.

Fixed the calls to the MaxMin selector to comply with the new api.
Fix call to the selector to comply with the new api.
Fix call to the selector
The example using mocked data was also fixed and the code cleaned
Small fixes were done so that the examples using `DirectedSphereExclusion`
could work. The lines with
`selector.select(coords_cluster, size=12,labels=class_labels_cluster)`
got broken with the last updates to the `DirectedSphereExclusion`. Now
the example only work if `labels=class_labels_cluster` is removed. It
is necessary to find why.
@marco-2023 marco-2023 requested a review from FarnazH July 17, 2023 04:28
@codecov
Copy link

codecov bot commented Jul 17, 2023

Codecov Report

Merging #147 (7ffe23c) into main (2a8ac47) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #147   +/-   ##
=======================================
  Coverage   40.54%   40.54%           
=======================================
  Files           8        8           
  Lines         555      555           
=======================================
  Hits          225      225           
  Misses        330      330           

@FanwangM
Copy link
Collaborator

Thanks for your efforts in building the notebook. @marco-2023
GitHub is preventing me from using the code review section because of the long text file from jupyter notebook. I will put comments below.

  • Would it be more precise to say "Running QC-Selector" instead of "Running Dissimilarity Algorithms" because we have multiple flavors for subset selection;
  • Do we have any explanations on why we get 8 and 6 data points instead of the queried 12 for "Adapted Optimizable K-Dissimilarity Selection (OptiSim)"?
  • Can we put all the imports at the top of the jupyter notebook?
  • For each subplot, can we add a title, x label and y label so that people don't have to scroll up to see what the figure means?
  • Can you add a few words saying why MaxSum is sensitive to outliers above the figure?
  • Why Directed Sphere Exclusion is giving 13 molecules instead of 12?

Sorry to put such a long list of questions, but I just want to also make it clear for some potential questions from the audience. Thanks.

@PaulWAyers
Copy link
Member

@marco-2023 I think this is good to go, probably, but it's good to respond to Fanwang's questions for documentation/bug-catching purposes.

@FanwangM FanwangM mentioned this pull request Aug 5, 2023
6 tasks
@FanwangM
Copy link
Collaborator

FanwangM commented Aug 5, 2023

Thank you for the nice work on this issue. I will merge this PR, but will leave a few more questions in #152.

@FanwangM FanwangM merged commit 7aeec91 into main Aug 5, 2023
5 of 8 checks passed
@xychem xychem mentioned this pull request Aug 18, 2023
@FanwangM FanwangM deleted the test_notebook branch November 2, 2023 18:51
JackyZzZz pushed a commit to JackyZzZz/Selector that referenced this pull request Jun 14, 2024
Update usages of the notebook because of the API changes
FanwangM added a commit that referenced this pull request Jul 2, 2024
Update usages of the notebook because of the API changes
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.

4 participants