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

add scoring filter #36

Merged
merged 15 commits into from
Dec 16, 2024
Merged

add scoring filter #36

merged 15 commits into from
Dec 16, 2024

Conversation

gcroci2
Copy link
Contributor

@gcroci2 gcroci2 commented Oct 14, 2024

In this PR I add the scoring filter component. Some comments:

  • A dictionary containing links data is now also saved in process_uploaded_data
  • At this stage I've implemented filtering in the scoring only for the metcalf method (the only one implemented in the NPLinker)
  • Added tests (and fixed the old ones) and docstrings

@gcroci2 gcroci2 self-assigned this Oct 14, 2024
@gcroci2 gcroci2 marked this pull request as draft October 14, 2024 13:54
@gcroci2 gcroci2 linked an issue Oct 14, 2024 that may be closed by this pull request
@github-advanced-security
Copy link

This pull request sets up GitHub code scanning for this repository. Once the scans have completed and the checks have passed, the analysis results for this pull request branch will appear on this overview. Once you merge this pull request, the 'Security' tab will show more code scanning analysis results (for example, for the default branch). Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results. For more information about GitHub code scanning, check out the documentation.

@gcroci2 gcroci2 requested a review from CunliangGeng December 6, 2024 17:38
@CunliangGeng
Copy link
Member

Is it ready for review?

@gcroci2
Copy link
Contributor Author

gcroci2 commented Dec 11, 2024

Is it ready for review?

It is not finished, but I have questions before I keep working on it (see the very first comment) @CunliangGeng

@CunliangGeng
Copy link
Member

Here are my thoughts on your questions:

Do we want to already have in place the OR/AND logic for the future methods? (in gm_scoring_apply) We can't really test it since we have only one possible scoring method atm

No, we only add them when applicable/needed. Also, we don't need to implement the options for rosetta scoring at the moment. So now we'll only implement the following part:
image
Please keep the selection of scoring methods possible though it's only one choice now.

How do we want to limit the scoring filter functionality? For example, we could disable it if there are no selected rows in the Data selection table above

Keeping it always enabled and wrapped would make interaction between data table and scoring filter easier.
When needed, users can unwrap it and set the options; otherwise, the default method and values are used.
If there is no selected rows, clicking Show spectra will show an warning message instead of the candidate links table.
image

@gcroci2 gcroci2 marked this pull request as ready for review December 13, 2024 13:57
@gcroci2 gcroci2 requested review from CunliangGeng and removed request for CunliangGeng December 13, 2024 13:58
Copy link
Member

@CunliangGeng CunliangGeng left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! Just one thing: better to move the button Set Scoring outside of scoring container and rename it as e.g. "Show results"? You could work on it in next PR.

@gcroci2
Copy link
Contributor Author

gcroci2 commented Dec 16, 2024

Looks great! Just one thing: better to move the button Set Scoring outside of scoring container and rename it as e.g. "Show results"? You could work on it in next PR.

Okay I've added this point to the next issue (#33)

@gcroci2 gcroci2 merged commit 07ecb9f into main Dec 16, 2024
6 checks passed
@gcroci2 gcroci2 deleted the 32_add_scoring_gcroci2 branch December 16, 2024 10:45
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.

Add the scoring part
2 participants