This repository was archived by the owner on Jan 23, 2025. It is now read-only.
-
Notifications
You must be signed in to change notification settings - Fork 11
Fix norm fn sigs #80
Open
zbohannan
wants to merge
20
commits into
r-bioinformatics:master
Choose a base branch
from
zbohannan:fix_norm_fn_sigs
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Fix norm fn sigs #80
Changes from all commits
Commits
Show all changes
20 commits
Select commit
Hold shift + click to select a range
945c571
Change fn sigs
zbohannan a2fdfb0
Harmonize TPM RPKM
zbohannan 53bff69
Start work on customdatastore
zbohannan 1721723
TPM fn sig harmonized to RPKM
zbohannan b925fa7
Address review issues
zbohannan 7008d8b
Created new rate fn for TPM and test
zbohannan 55c7e26
Minor cleanup
zbohannan 9121664
Fix minor errors
zbohannan eba0419
Fix PR issues, add fn to get counts per base
zbohannan 4f88681
Add kb->b conversion
zbohannan 066f915
Adjust TPM for length conversion
zbohannan e1acb5f
Testing kb conversion
zbohannan 11653f9
Add get_rates test v1
zbohannan 166ab5c
Test get_rates v2
zbohannan d94bdbc
Test get_rates v3
zbohannan 36af95e
Add get rates test v4
zbohannan 77418a6
Add get rates test v5
zbohannan 7da1f8f
Add get rate test v6
zbohannan 8295ac9
Reformat
zbohannan 3369930
Reformat
zbohannan File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,3 @@ | ||
|
||
# -*- coding: utf-8 -*- | ||
# | ||
# Configuration file for the Sphinx documentation builder. | ||
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is similar to how I designed the the test_tpm in the first run. Here is the comment by clintval to that:
2276d19#r224543989
That's the reason why I chose a very simple table that is manually calculatable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess I'm a bit confused. Is this in reference to the comments about using random values? This test shouldn't be using anything random; it should be pulling the first entry from the test data, similar to what the RPKM test is doing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The link I posted is not working as I expected! I'll copy the comment here:
clintval: "Not bad, you are dynamically creating an expected result by copying some code from the main application.
This makes it hard to track down bugs with the logic since the logic exists in two places.
Norm. methods should probably be tested with a very small, hand-designed test case so you can explicitly write out all inputs, and all outputs."
This is why I didn't implement the test_tpm function like the test_rpkm function in the end. As a consequence also test_rpkm shouldn't be implemented as is at the moment..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, that makes much more sense. In that case, it may make sense to us apfejes' RPKM function sig and your test function as standards. It seems reasonable but probably worth discussing (unless it's already been discussed before I arrived).
Would it be worth it to possibly create a minimal test data set and run it through edgeR to match the edgeR outputs to ours? That's probably a larger discussion, but known and verified 3rd-party outputs may make sense for test cases.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider too, an additional layer of testing as property-based for these mathematical calculations:
https://hypothesis.readthedocs.io/en/latest/quickstart.html#writing-tests