Skip to content

fix vignettes, refactor chromBackendSpectra#44

Merged
philouail merged 10 commits intomainfrom
phili
Jan 22, 2026
Merged

fix vignettes, refactor chromBackendSpectra#44
philouail merged 10 commits intomainfrom
phili

Conversation

@philouail
Copy link
Collaborator

No description provided.

@philouail philouail requested a review from jorainer January 21, 2026 12:37

The number of chromatograms in the `Chromatograms` object is reduced.

Note that for `ChromBackendSpectra`, when you subset the `Chromatograms` object,
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

i might remove this, realizing now im overexplaining a bit this whole index thing..

Copy link
Member

Choose a reason for hiding this comment

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

I would keep it.

Copy link
Member

@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.

Some minor things, suggestions and comments.

Also, a more general discussion:

  1. Chromatograms are per definition sorted by retention time, right? and that is also explained prominently (in the documentation and vignette)?

  2. I'm wondering whether it would not be easier for the Spectra backend to order the results of any calculation/extraction of intensity/retention time values than ordering the actual Spectra object? the results should be numeric vectors of rtime and intensity values and ordering them would be less expensive compared to ordering a Spectra object.

@philouail
Copy link
Collaborator Author

  1. Chromatograms are per definition sorted by retention time, right? and that is also explained prominently (in the documentation and vignette)?

mm do you mean the peaksData of a single chromatogram ? because that's yes.

@philouail philouail requested a review from jorainer January 21, 2026 14:44
Copy link
Member

@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.

Really nice work @philouail ! I have only some minor comments and suggestions.

#' all data into memory.
#'
#' **Factorize and Subsetting**: The `factorize()` method updates the
#' `chromSpectraIndex` in both `chromData` and the `spectra` object to reflect
Copy link
Member

Choose a reason for hiding this comment

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

If you mean the actual Spectra object you should write Spectra, not spectra. If you mean the slot, you could actually also write @spectra or write that it's the Spectra stored in the internal @spectra slot? Actually, the same/similar thing maybe also for the chromData - and all other internal slots

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ok i went through the package and tried to follow this.


The number of chromatograms in the `Chromatograms` object is reduced.

Note that for `ChromBackendSpectra`, when you subset the `Chromatograms` object,
Copy link
Member

Choose a reason for hiding this comment

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

I would keep it.

@philouail philouail merged commit 42480ad into main Jan 22, 2026
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