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

Adding chromPeaks metadata to the Spectra output of chromPeakSpectra() #779

Merged
merged 19 commits into from
Dec 15, 2024

Conversation

philouail
Copy link
Collaborator

@jorainer Just wanted to get your opinion on this.. Because appending multiple column at once to the spectraData() of an object is not possible. i only see 2 options:

  • accessing and manipulating the spectraData slot ( in this PR)
  • a for loop adding columns one by ones to the res object.

Am I missing an easier way to deal with this ?

This PR is not ready, just wanted to get your opinion before I also update featureSpectra() and the documentation.

@philouail philouail requested a review from jorainer November 20, 2024 16:50
Copy link
Collaborator

@jorainer jorainer left a comment

Choose a reason for hiding this comment

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

this will be a very useful and helpful addition Phili! thanks! but there are some more general thing we can/should discuss

@philouail philouail requested a review from jorainer November 21, 2024 21:34
@philouail
Copy link
Collaborator Author

@jorainer do you know what's happening with the Linux checks ? i get a weird error, did I do something wrong ?

@philouail philouail marked this pull request as ready for review November 22, 2024 09:24
Copy link
Collaborator

@jorainer jorainer left a comment

Choose a reason for hiding this comment

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

Looks good to me, thanks Phili! Only thing is that I find the parameter names a bit bulky - addColumnsChromPeaks and addColumnsFeatures - I would prefer either chromPeakColumn/featureColumns or addChromPeakColumns/addFeatureColumns.

Also, I think you were right with the prefix. Maybe we don't expose that parameter in the function - I would still use it internally, so we can re-add it as a function parameter if needed, but for now I would remove it from the function definition.

Copy link
Collaborator

@jorainer jorainer left a comment

Choose a reason for hiding this comment

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

Thanks for adapting, but now I found some additional issues that might cause problems later, i.e. it would be key to check that the requested columns are available. sorry that I missed that the previous time.

@philouail philouail requested a review from jorainer November 22, 2024 15:22
Copy link
Collaborator

@jorainer jorainer left a comment

Choose a reason for hiding this comment

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

Thanks Phili! Looks great! Only very minor thing: please bump the version to 4.5.2.

@sneumann , would be OK for me to merge into devel and push to BioC

@sneumann
Copy link
Owner

looks great, merging and pushing now. Yours, Steffen

@sneumann sneumann merged commit 143f3b9 into devel Dec 15, 2024
2 of 3 checks passed
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