search

sneumann / xcms

This is the git repository matching the Bioconductor package xcms: LC/MS and GC/MS Data Analysis
Other
177 stars 81 forks source link

fix issue #716 in XcmsExperiment-functions.R #719

Closed pablovgd closed 4 months ago

pablovgd commented 4 months ago

As mentioned in issue #716: I tried to implement a fix that makes sure the sn column also appears when using the manualChromPeaks function so that other function to extract and plot the XChromatograms objects work. Noise estimation happens using the estimateChromNoise function, feel free to suggest better ways to implement this.

jorainer commented 4 months ago

Thanks @pablovgd ! I'll have a look into this. it might be safer to introduce/require the column further upstream, but I'll check that.

jorainer commented 4 months ago

Ah, and if that's your first pull request - just make changes in your fork and push them (to your fork), this will automatically update this PR. And click on "Re-request Review" on the right in the "Reviewers" panel to re-request my review.

pablovgd commented 4 months ago

Thanks for the feedback Johannes! I'll take a look to make the changes you requested and will re-request review when ready.

pablovgd commented 4 months ago

Alright, I think I managed to implement all changes you requested. Thanks again for the feedback.

jorainer commented 4 months ago

Ah @pablovgd , and one last change request: please add yourself as a contributor ("ctb") to the DESCRIPTION file. The bug fix along with the unit tests qualifies you as a contributor following the RforMS contribution guidelines.

pablovgd commented 4 months ago

@jorainer Ah, great, done!

sneumann commented 4 months ago

Thanks everyone, merging now and pushing to BioC. Yours, Steffen

sneumann commented 4 months ago

Hi, before pushing to BioC I ran a local `R CMD check: and got:

  Running ‘testthat.R’
 ERROR
Running the tests in ‘tests/testthat.R’ failed.
Last 13 lines of output:
   - maxPeakwidth: [1] 13.2
  Object of class:  MergeNeighboringPeaksParam 
...
  Error in parse(con, n = -1, srcfile = srcfile, encoding = "UTF-8") : 
    test_XcmsExperiment.R:1001:5: unexpected symbol
  1001:     res2
            ^
  Calls: test_check ... doTryCatch -> lapply -> FUN -> source_file -> parse
  Execution halted

and there seems to be a missing closing bracket in https://github.com/sneumann/xcms/blob/b1ae81942dad37169c21a6c717d4ce7480b64c07/tests/testthat/test_XcmsExperiment.R#L999 So we should also check the output of the GitHub actions: https://github.com/sneumann/xcms/actions/runs/7957720356/job/21721103315#step:21:652

I fixed the missing bracket, working now, and pushed to BioC. Yours, Steffen

jorainer commented 4 months ago

Ah, oh, I'll also check and eventually fix.

jorainer commented 4 months ago

Ah, didn't see that you already fixed. can only confirm that it works now.