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

Tune unit tests (issue #333) #334

Merged
merged 3 commits into from
Apr 13, 2018
Merged

Tune unit tests (issue #333) #334

merged 3 commits into from
Apr 13, 2018

Conversation

jorainer
Copy link
Collaborator

See issue #333 for details about this PR. Performance of unit tests was improved by sub-setting large data files.

Copy link
Owner

@lgatto lgatto left a comment

Choose a reason for hiding this comment

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

A general comment. Instead of repeating filterRt(ondisk1, c(1150, 1200)) (and similar), what about defining ondisk1_re_1150_1200 (and similar) in testthat.R (where ondisk1 et al. are defined) and reuse it later?

k = 1)
## Reduce the TMT erwinia data set to speed up processing on the full
## data.
tmt <- filterMz(filterRt(tmt_erwinia_in_mem_ms1, c(1200, 1250)),
Copy link
Owner

Choose a reason for hiding this comment

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

How much do you gain from sub-setting along M/Z? I would keep it so that we actually pick peaks along the whole M/Z range. Filtering along the retention time makes sense, as we repeat the same operation many times.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You're right. For MSnExp we probably don't gain much. I'll keep that to filterRt and also define the filtered objects in testthat.R as you suggest.

@codecov-io
Copy link

codecov-io commented Apr 13, 2018

Codecov Report

Merging #334 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff           @@
##           master    #334   +/-   ##
======================================
  Coverage    76.7%   76.7%           
======================================
  Files          79      79           
  Lines        8642    8642           
======================================
  Hits         6629    6629           
  Misses       2013    2013

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a3650a0...14bfe6c. Read the comment docs.

@lgatto lgatto merged commit a182c6e into master Apr 13, 2018
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.

3 participants