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

Add an exporter for ASICS; provide an autophase library wrapper #68

Closed
wants to merge 4 commits into from

Conversation

NeutralKaon
Copy link
Contributor

@NeutralKaon NeutralKaon commented Jan 19, 2024

Hi there

Thanks for making AlpsNMR – it has been hugely useful to me

This is (very pretentiously) a commit to provide two helper functions for other R packages; ASICS -- a spectral quantification library -- and and NMRphasing, which provides a variety of 1D NMR phasing (and integrated baseline correction) methods (which I'm going to email the developer and ask him to make optional). Very rudely I've added myself as a contributor – feel free to delete if you don't think that's appropriate.

I've written documentation and tests for them, and also tried to ameliorate #66 by adding literally half a sentence in a function's description there too.

@zeehio
Copy link
Member

zeehio commented Jan 19, 2024

Thank you very much for your contribution.

I will do my best to find some time to review and test it, as well as check with other authors for their feedback and then merge this

@zeehio zeehio self-assigned this Jan 19, 2024
@NeutralKaon
Copy link
Contributor Author

Thanks so much – I have no idea why it doesn't pass the R CMD CHECK workflow – it does if I build source packages and then try them separately. It is far from a big contribution! ;-)

@zeehio
Copy link
Member

zeehio commented Jan 21, 2024

Thanks so much – I have no idea why it doesn't pass the R CMD CHECK workflow – it does if I build source packages and then try them separately. It is far from a big contribution! ;-)

Don't worry it could be that the CI has some other issues. I'll check and fix it.

@zeehio
Copy link
Member

zeehio commented Jun 9, 2024

I have taken your commits and included them in the main branch.

I have made some performance improvements with BiocParallell and changed the function names.

This is the latest version of to_ASICS:

AlpsNMR/R/utils.R

Lines 295 to 299 in 9b1d566

to_ASICS <- function(dataset, ...) {
require_pkgs("ASICS")
spectra_matrix <- t(nmr_data(dataset))
ASICS::createSpectra(spectra_matrix, ...)
}

And here you have the nmr_autophase:

https://github.com/sipss/AlpsNMR/blob/9b1d5665187cba645dab1e2fb745115058adb7b8/R/nmr_autophase.R

Your autophase wrapper was passing the whole dataset object to each of the BiocParallel workers. The current version passes to each worker only the spectra it needs to process, reducing the memory usage significantly in smaller datasets.

I also included some warnings and made sure examples could run.

Since your contribution is now merged I will close this pull request. Thank you!

@zeehio zeehio closed this Jun 9, 2024
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.

None yet

2 participants