wincowgerDEV / OpenSpecy-package

Analyze, Process, Identify, and Share, Raman and (FT)IR Spectra
http://wincowger.com/OpenSpecy-package/
Creative Commons Attribution 4.0 International
23 stars 11 forks source link

V1.0 prep #129

Closed wincowgerDEV closed 12 months ago

wincowgerDEV commented 1 year ago

@zsteinmetz, creating this PR so that we can have everything in one place. Below I am bringing over comments and check boxes from the other 3 PRs and assigning each of us to tasks. Would it be alright if I merge all these to V1.0 prep?

Package Down Documentation (@wincowgerDEV) https://github.com/wincowgerDEV/OpenSpecy-package/pull/126

Functions for managing spectra for v1.0 (@zsteinmetz) https://github.com/wincowgerDEV/OpenSpecy-package/pull/125

Reading functions for v1.0 (@zsteinmetz) https://github.com/wincowgerDEV/OpenSpecy-package/pull/124

Functions to be renamed/changed (@zsteinmetz)

General checks and CRAN compliance (@zsteinmetz)

wincowgerDEV commented 1 year ago

Checking all these R files for completion and tests:

wincowgerDEV commented 1 year ago

@zsteinmetz and @nickleong20 I have gone through everything now besides the run_app function. I think it is broken because the app hasn't been updated with the new functions yet so I am going to pivot to work on that next. @zsteinmetz do you know why the automated checks are failing? I can seem to figure out how to fix them.

zsteinmetz commented 1 year ago

Awesome work, @wincowgerDEV and @nickleong20! :heart_eyes:

I think it is broken because the app hasn't been updated with the new functions yet so I am going to pivot to work on that next.

Yeah! We'll make run_app() work once the Shiny app is up to speed again. Shouldn't be a big deal.

@zsteinmetz do you know why the automated checks are failing? I can seem to figure out how to fix them.

The workflow version we've been using is now deprecated. I can bump it to the next version when I'm reviewing the code next week. Just did the same with my other package.

wincowgerDEV commented 1 year ago

Yup, totally agree to add Nick!

On Fri, Jul 28, 2023 at 11:10 AM Zacharias Steinmetz < @.***> wrote:

@.**** commented on this pull request.

In DESCRIPTION https://github.com/wincowgerDEV/OpenSpecy-package/pull/129#discussion_r1277930451 :

@@ -1,13 +1,13 @@ Package: OpenSpecy Type: Package Title: Analyze, Process, Identify, and Share Raman and (FT)IR Spectra -Version: 0.9.5 -Date: 2022-07-06 +Version: 1.0.0 +Date: 2023-07-28 @.***: c(person("Win", "Cowger", role = c("cre", "aut"),

We'll need to add @nickleong20 https://github.com/nickleong20 here, won't we?

— Reply to this email directly, view it on GitHub https://github.com/wincowgerDEV/OpenSpecy-package/pull/129#pullrequestreview-1552681136, or unsubscribe https://github.com/notifications/unsubscribe-auth/AGMUJU75ME6BYHCFN5KNEMLXSP57XANCNFSM6AAAAAA2C62GJI . You are receiving this because you were mentioned.Message ID: @.***>

--

´¯·.¸¸.·´¯·.´¯·.¸¸.·´¯ツ ------------------------------

Win Cowger, PhD Pronouns: he/him Research Scientist Moore Institute for Plastic Pollution Research

Contact Info

515-298-3869 | @.*** | @Win_OpenData https://twitter.com/Win_OpenData

Websites Personal Website: www.wincowger.com Currently Employed: https://mooreplasticresearch.org/ Alumni Of: https://www.thegraylab.org/ Project Websites: www.openspecy.org Research Gate: https://www.researchgate.net/profile/Win-Cowger Github: https://github.com/wincowgerDEV OSF: https://osf.io/kxeh5/

wincowgerDEV commented 1 year ago

Good point, may want to implement ourselves just so we don't have to back them up or wait.

On Fri, Jul 28, 2023 at 11:16 AM Zacharias Steinmetz < @.***> wrote:

@.**** commented on this pull request.

In R/read_multi.R https://github.com/wincowgerDEV/OpenSpecy-package/pull/129#discussion_r1277937445 :

  • ignore.case = T, file)) {
  • ex <- strsplit(basename(file), split="\.")[[1]][-1]
  • os <- do.call(paste0("read_", tolower(ex)), list(file = file, ...))
  • } else if (grepl("(\.zip$)", ignore.case = T, file)) {
  • os <- read_zip(file = file, ...)
  • } else {
  • os <- read_spec(file = file, ...)
  • }
  • return(os) +}
  • +#' @rdname read_multi +#' +#' @importFrom utils unzip +#' @importFrom hySpc.read.ENVI read_ENVI_Nicolet

If we want to submit v1.0.0 to CRAN, we can't use the hySpc.read.ENVI package as long it's not on CRAN itself. We could either motivate them to submit soon or implement those functions on our own.

— Reply to this email directly, view it on GitHub https://github.com/wincowgerDEV/OpenSpecy-package/pull/129#pullrequestreview-1552691527, or unsubscribe https://github.com/notifications/unsubscribe-auth/AGMUJU2EI5JPZ6OBKIOCWC3XSP6Y7ANCNFSM6AAAAAA2C62GJI . You are receiving this because you were mentioned.Message ID: @.***>

--

´¯·.¸¸.·´¯·.´¯·.¸¸.·´¯ツ ------------------------------

Win Cowger, PhD Pronouns: he/him Research Scientist Moore Institute for Plastic Pollution Research

Contact Info

515-298-3869 | @.*** | @Win_OpenData https://twitter.com/Win_OpenData

Websites Personal Website: www.wincowger.com Currently Employed: https://mooreplasticresearch.org/ Alumni Of: https://www.thegraylab.org/ Project Websites: www.openspecy.org Research Gate: https://www.researchgate.net/profile/Win-Cowger Github: https://github.com/wincowgerDEV OSF: https://osf.io/kxeh5/

zsteinmetz commented 1 year ago

Yup, totally agree to add Nick!

@nickleong20: Do you have an ORCID?

nickleong20 commented 1 year ago

Yup, totally agree to add Nick!

@nickleong20: Do you have an ORCID?

@zsteinmetz Yes, it is 0009-0008-3313-4132. Thank you!

zsteinmetz commented 1 year ago

devtools::check() running again, currently with:

2 errors ✖ | 4 warnings ✖ | 3 notes ✖

Mostly coming from missing documentation and some rogue tests. I'll go through it step by step.

wincowgerDEV commented 1 year ago

@nickleong20 and @zsteinmetz, was thinking about a small patch this morning to the as_OpenSpecy function but want to pass it by you first to make sure you agree and that it won't likely cause some huge crash. After going through all of the functions I realized that there are a few assumptions that the functions make which we might be able to help ensure happen correctly or at least provide a test through is_OpenSpecy to make sure it works.

  1. One is that the wavenumber range is continuous, if the wavenumber range is scrambled stuff like baseline correction won't work out of the box. I think I could add a reorder function to as_OpenSpecy just to ensure that the wavenumbers will be ordered for all the functions. Maybe we force wavenumbers to be ordered from greatest to least?

  2. Another is that the column names in OpenSpecy$spectra are unique because we currently allow people to filter and sort the Open Specy objects by column name. We could make a check to see if the column names are unique, if they are not then we digest the spectral intensities for each spectrum and use that as the column name and add that to the metadata. There could also be a scenario where people have uploaded duplicate spectra and those should actually be collapsed, but then I can also imagine some people wanting to be lazy with their analysis and just allowing duplicates for the things they are doing and working around it.

Let me know what yall think. Win

wincowgerDEV commented 1 year ago

@zsteinmetz Update for the week, I think we are getting super close. I got the app working again with all the new features we added, really happy with its functionality, still some aesthetic bugs but W/E. Would it be alright with you if I merge that PR for the app now? I think that will make finishing the run_app function easier and I think it is pretty stable as is as long as the user is using our updated package. I spoke with @nickleong20 about the comment above and I think for now we just provide a warning message for those and we can think about more streamlined requirements later if users run into common bugs there.

@nickleong20 went through all the functions and tests and so did I and Nick is currently working on:

After we get the package finalized and submitted to cran I will do the packagedown integrations to build out our website.

zsteinmetz commented 1 year ago

@nickleong20 and @zsteinmetz, was thinking about a small patch this morning to the as_OpenSpecy function but want to pass it by you first to make sure you agree and that it won't likely cause some huge crash. After going through all of the functions I realized that there are a few assumptions that the functions make which we might be able to help ensure happen correctly or at least provide a test through is_OpenSpecy to make sure it works.

Very good point! I'll comment directly on your code ..

zsteinmetz commented 1 year ago

@zsteinmetz Update for the week, I think we are getting super close. I got the app working again with all the new features we added, really happy with its functionality, still some aesthetic bugs but W/E. Would it be alright with you if I merge that PR for the app now? I think that will make finishing the run_app function easier and I think it is pretty stable as is as long as the user is using our updated package. I spoke with @nickleong20 about the comment above and I think for now we just provide a warning message for those and we can think about more streamlined requirements later if users run into common bugs there.

You're speaking of https://github.com/wincowgerDEV/OpenSpecy-shiny/pull/21, right? Fine with me to merge :+1:

This PR here needs a bit more tweaking though before you can merge. I really like to see all tests passing before it goes into main.

zsteinmetz commented 1 year ago

We're now at:

1 error ✖ | 4 warnings ✖ | 3 notes ✖

Running all the examples is rather resource intensive; see:

   Examples with CPU (user + system) or elapsed time > 5s
                     user system elapsed
   plot_OpenSpecy  19.506  0.272  20.075
   process_spectra  5.439  0.010   5.488

We should try to run less and more lightweight examples.

zsteinmetz commented 1 year ago

@wincowgerDEV and @nickleong20, I was thinking about renaming some of the functions to make them more consistent throughout the package. For instance, we often abbreviated "spectra" with "spec" in function names but "process_spectra" has the full name. What's your take on this? I would, of course, provide you with a list of all changed function names.

zsteinmetz commented 1 year ago

Down to

1 error ✖ | 2 warnings ✖ | 1 note ✖
wincowgerDEV commented 1 year ago

@wincowgerDEV and @nickleong20, I was thinking about renaming some of the functions to make them more consistent throughout the package. For instance, we often abbreviated "spectra" with "spec" in function names but "process_spectra" has the full name. What's your take on this? I would, of course, provide you with a list of all changed function names.

I think go for it, you're right the names new to be consistent for people to easily use the package, should be too much work to change the function names in our SOPs and the app. Also noticed some places the first object is named object and some places it's named x. Perhaps we should make that consistent too.

zsteinmetz commented 1 year ago

Also noticed some places the first object is named object and some places it's named x. Perhaps we should make that consistent too.

I already stumbled upon it. Working on making those consistent as well.

zsteinmetz commented 1 year ago
  • characterize_particles() -> ?

Should we keep calling it "particles"? The function might not be restricted to particles but could also be used for fragments, fibers, or whatever form, right? What about using the term "item" or "feature"?

wincowgerDEV commented 1 year ago
  • characterize_particles() -> ?

Should we keep calling it "particles"? The function might not be restricted to particles but could also be used for fragments, fibers, or whatever form, right? What about using the term "item" or "feature"?

I like characterize_feature, I guess you could use the function to even separate regions on a single particle if you wrote the logical statement that way.

zsteinmetz commented 1 year ago

I like characterize_feature

.. and what about "characterize"? We could also use "define" or "ident" .. maybe that would make it a bit clearer? :thinking:

wincowgerDEV commented 1 year ago

.. and what about "characterize"? We could also use "define" or "ident" .. maybe that would make it a bit clearer? 🤔

That function primarily measures the particles and gives them unique IDs. I think either define or identify could work. There is also an identify_spec function so we may want to do define so it isn't confused with the other function.

zsteinmetz commented 1 year ago

Btw, tests are running way too long. We shouldn't download the complete library for testing purposes. The tests are currently the last remaining error :partying_face:

1 error ✖ | 0 warnings ✔ | 0 notes ✔

Working on this right now.

wincowgerDEV commented 1 year ago

Btw, tests are running way too long. We shouldn't download the complete library for testing purposes. The tests are currently the last remaining error 🥳

1 error ✖ | 0 warnings ✔ | 0 notes ✔

Working on this right now.

You're a legend bro! Yeah agree that we shouldn't download the whole library every time, maybe we can move that test to a place where it is only tested on an as-needed basis? Then just test with the test_lib internal dataset?

zsteinmetz commented 1 year ago

You're a legend bro! Yeah agree that we shouldn't download the whole library every time, maybe we can move that test to a place where it is only tested on an as-needed basis? Then just test with the test_lib internal dataset?

Yeah, I was thinking about something similar. Looking into it tomorrow!

zsteinmetz commented 1 year ago

We're going to need a new test library on OSF, right? One that follows the same naming convention as the other new libraries; something like both_test.rds. The data structure should be the same as in both_derivative.rds containing both Raman and FTIR spectra. At the moment, it's Raman only, I guess. A subset of 200-300 spectra and less than 1 MB total file size would be ideal. Is there an easy way producing something like this?

wincowgerDEV commented 1 year ago

The easiest way would be to use the sample_spec and/or filter_spec functions on the both_derivative library.

On Thu, Aug 10, 2023, 3:40 AM Zacharias Steinmetz @.***> wrote:

We're going to need a new test library on OSF, right? One that follows the same naming convention as the other new libraries; something like both_test.rds. The data structure should be the same as in both_derivative.rds containing both Raman and FTIR spectra. At the moment, it's Raman only, I guess. A subset of 200-300 spectra and less than 1 MB total file size would be ideal. Is there an easy way producing something like this?

— Reply to this email directly, view it on GitHub https://github.com/wincowgerDEV/OpenSpecy-package/pull/129#issuecomment-1672980562, or unsubscribe https://github.com/notifications/unsubscribe-auth/AGMUJUZBU46FAAHCYRXX3DLXUS3ATANCNFSM6AAAAAA2C62GJI . You are receiving this because you were mentioned.Message ID: @.***>

zsteinmetz commented 1 year ago

Any recommendation which spectra to use?

wincowgerDEV commented 1 year ago

I guess making sure there are examples for each plastic type could be good, that way it can be used easily for our primary use case.

On Thu, Aug 10, 2023, 6:12 AM Zacharias Steinmetz @.***> wrote:

Any recommendation which spectra to use?

— Reply to this email directly, view it on GitHub https://github.com/wincowgerDEV/OpenSpecy-package/pull/129#issuecomment-1673194709, or unsubscribe https://github.com/notifications/unsubscribe-auth/AGMUJU7Q3IX5HLPC6S3Q3RTXUTM4NANCNFSM6AAAAAA2C62GJI . You are receiving this because you were mentioned.Message ID: @.***>

zsteinmetz commented 1 year ago

Do you recall how you did it last time?

wincowgerDEV commented 1 year ago

Do you recall how you did it last time?

@zsteinmetz

Just went ahead and created it here: https://osf.io/x7dpz/

Basically, I ran this:

write_spec(
filter_spec(library, distinct(library$metadata, SpectrumType, polymer_class, .keep_all =T)$sample_name), 
"both_test.rds")
wincowgerDEV commented 1 year ago

Good catch. Make rel should be applied to each spectrum separately as a general rule. I can imagine some fringe use cases where we want it to apply to everything at once but I wouldn't plan for them.

On Tue, Aug 15, 2023, 3:08 AM Zacharias Steinmetz @.***> wrote:

@.**** commented on this pull request.

In R/adj_range.R https://github.com/wincowgerDEV/OpenSpecy-package/pull/129#discussion_r1294416158 :

  • if (make_rel) x$spectra <- make_rel(filt) else x$spectra <- filt
  • return(x) +}
  • +#' @rdname adj_range +#' +#' @export +flatten_range <- function(x, ...) {

  • UseMethod("flatten_range") +}
  • +#' @rdname adj_range +#' +#' @export +flatten_range.default <- function(x, ...) {

  • stop("object 'x' needs to be of class 'OpenSpecy'") +}
  • +#' @rdname adj_range +#' +#' @export +flatten_range.OpenSpecy <- function(x, min_range, max_range, make_rel = TRUE,

  • ...) {
  • if(length(min_range) != length(max_range)) {
  • stop("min_range and max_range need to be the same length")
  • }
  • if(any(vapply(1:length(min_range), function(y) {
  • min_range[y] > max_range[y]
  • }, FUN.VALUE = logical(1)))) {
  • stop("all min_range values must be lower than corresponding max_range")
  • }
  • filt <- x$spectra[,lapply(.SD, function(y) {
  • .flatten_range(wavenumber = x$wavenumber,
  • spectra = y,
  • min_range = min_range,
  • max_range = max_range)
  • })]
  • if (make_rel) x$spectra <- filt[, lapply(.SD, make_rel)] else x$spectra <- filt

Is make_rel() supposed to normalize each spectrum in $spectra individually or should they be normalized together? Currently we have both types here and they differ between restrict_range() (L74) flatten_range() (L113).

— Reply to this email directly, view it on GitHub https://github.com/wincowgerDEV/OpenSpecy-package/pull/129#pullrequestreview-1578294460, or unsubscribe https://github.com/notifications/unsubscribe-auth/AGMUJU6KG2YHPM5TMQL63I3XVNDAPANCNFSM6AAAAAA2C62GJI . You are receiving this because you were mentioned.Message ID: @.***>

wincowgerDEV commented 1 year ago

Hey @zsteinmetz, thanks for all the hard work on this! Commenting on the last todos: agree with your updates to the two functions. I think we can keep corspec slow for now. I'm working on that a ton and we may update the package later with some improvements to that function or create new functions that are faster (probably better to avoid breaking). I can add the secrets to github soon. Now that we have the function names and variables settled I can go through the shiny app and make it stable again and push the package down updates.

zsteinmetz commented 1 year ago

I can add the secrets to github soon.

Hey @wincowgerDEV, sorry to bother you with the secrets again :upside_down_face: Could you maybe add them to the repo that I can adjust the workflow?

wincowgerDEV commented 1 year ago

Hey Zacharias,

Sorry about that, just added them!

Warm Regards, Win

On Tue, Aug 22, 2023 at 1:43 AM Zacharias Steinmetz < @.***> wrote:

I can add the secrets to github soon.

Hey @wincowgerDEV https://github.com/wincowgerDEV, sorry to bother you with the secrets again 🙃 Could you maybe add them to the repo that I can adjust the workflow?

— Reply to this email directly, view it on GitHub https://github.com/wincowgerDEV/OpenSpecy-package/pull/129#issuecomment-1687744917, or unsubscribe https://github.com/notifications/unsubscribe-auth/AGMUJU3XK4ALMPYEAXSOIS3XWRWMDANCNFSM6AAAAAA2C62GJI . You are receiving this because you were mentioned.Message ID: @.***>

--

´¯·.¸¸.·´¯·.´¯·.¸¸.·´¯ツ ------------------------------

Win Cowger, PhD Pronouns: he/him Research Scientist Moore Institute for Plastic Pollution Research

Contact Info

515-298-3869 | @.*** | @Win_OpenData https://twitter.com/Win_OpenData

Websites Personal Website: www.wincowger.com Currently Employed: https://mooreplasticresearch.org/ Alumni Of: https://www.thegraylab.org/ Project Websites: www.openspecy.org Research Gate: https://www.researchgate.net/profile/Win-Cowger Github: https://github.com/wincowgerDEV OSF: https://osf.io/kxeh5/

zsteinmetz commented 1 year ago

Hey Zacharias, Sorry about that, just added them! Warm Regards, Win

Thanks, man!

Whoops, doesn't seem to work out of the box. Fixed now.

wincowgerDEV commented 1 year ago

From my end, we're good to go once the SOP and run_app() are updated. The latter shouldn't be a big deal. Let me know if I can help with the Shiny app.

Right on man! I will spend some time on the shiny app this week and @nickleong20 will work on the SOP after that. Should be ready to roll in the next few weeks. I definitely never pass up an opportunity to get your input, maybe I will run through the shiny app update first and once I have it stable from my perspective you could take another look before we announce the full update?

zsteinmetz commented 1 year ago

Good news! I found a way how to add non-CRAN repos like https://r-hyperspec.github.io/pkg-repo/ to the OpenSpecy package: https://stackoverflow.com/questions/33335321/include-non-cran-package-in-cran-package

They can only go into Suggests, so no hard dependencies, but still. This might be a way to work with future versions of the hySpc.* family to avoid forking all their code. Nothing for v1.0.0, I guess, but something to keep in mind; especially if they fix issues like https://github.com/wincowgerDEV/OpenSpecy-shiny/issues/30 ...

wincowgerDEV commented 1 year ago

Righteous!

That's definitely helpful. I've been noticing an uptick in never-CRAN packages lately that choose not to submit to CRAN because they break too many of the rules but they are really good at specific operations so this will definitely open some doors there.

On Wed, Aug 23, 2023, 6:35 AM Zacharias Steinmetz @.***> wrote:

Good news! I found a way how to add non-CRAN repos like https://r-hyperspec.github.io/pkg-repo/ to the OpenSpecy package. They can only go into Suggests, so no hard dependencies, but still. This might be a way to work with future versions of the hySpc.* family to avoid forking all their code. Nothing for v1.0.0, I guess, but something to keep in mind; especially if they fix issues like wincowgerDEV/OpenSpecy-shiny#30 https://github.com/wincowgerDEV/OpenSpecy-shiny/issues/30 ...

— Reply to this email directly, view it on GitHub https://github.com/wincowgerDEV/OpenSpecy-package/pull/129#issuecomment-1689978785, or unsubscribe https://github.com/notifications/unsubscribe-auth/AGMUJU7KVXUIGBWRITFCWZTXWYBKBANCNFSM6AAAAAA2C62GJI . You are receiving this because you were mentioned.Message ID: @.***>

zsteinmetz commented 1 year ago

I've been noticing an uptick in never-CRAN packages lately that choose not to submit to CRAN because they break too many of the rules but they are really good at specific operations so this will definitely open some doors there.

Just to manage expectations: there still needs to be some sort of CRAN-like repo such as https://r-hyperspec.github.io/pkg-repo/; a GitHub repo won't work.

wincowgerDEV commented 1 year ago

There are a few errors on my computer when I run the tests, going to work on these before I get running with the shiny app:

[ FAIL 4 | WARN 0 | SKIP 5 | PASS 330 ]

wincowgerDEV commented 1 year ago

Looks like expect_contains is throwing things off, may need to update testthat

zsteinmetz commented 1 year ago

Are you running the latest testthat version? expect_contains() has been introduced just lately.

wincowgerDEV commented 1 year ago

All clear!

wincowgerDEV commented 1 year ago

Are you running the latest testthat version? expect_contains() has been introduced just lately.

Just updated the testthat and everything is working :)

zsteinmetz commented 1 year ago

Just added this specific requirement to the package description.

wincowgerDEV commented 1 year ago

Just added this specific requirement to the package description.

Fasho! Good call.

Do you know where a pdf file might be created during the tests? Currently getting a pdf with images of spectra in it called Rplots in the testthat folder when I run the tests.

zsteinmetz commented 1 year ago

Do you know where a pdf file might be created during the tests? Currently getting a pdf with images of spectra in it called Rplots in the testthat folder when I run the tests.

Dunno; saw it once but it didn't reappear when I deleted it.

wincowgerDEV commented 1 year ago

Dunno; saw it once but it didn't reappear when I deleted it.

Fasho, It appears to be something with the plot.OpenSpecy because the plots are the same but not totally positive, it's low priority, only occurs when the tests are run for me and will be occurring in a temp environment on cran so I think it is fine. Will proceed with the shiny app dev.

wincowgerDEV commented 1 year ago

Updated the app now with the new functions and the run_app function is working but I don't have a way to test that the app actually runs yet, just testing some silly logic for the test. @zsteinmetz if you have time to check the app and run_app function that would be great but also I am down to just submit to CRAN. I think the community will end up fleshing out any remaining bugs for us and we can debug as we go.

zsteinmetz commented 1 year ago

Wow, that was fast! :heart_eyes:

Automatically testing Shiny apps as a whole is not as easy as testing package functions; one of the many reasons why packages underneath an app are really helpful. There are some shinytest packages but I haven't looked into them yet and I guess it would make more sense to add them to the Shiny app itself rather than to the package.

What I did now is adding some test logic to run_app() so that all function code is tested but the last line running the actual app. With this, we make sure that everything is in order from the package end and keep everything else to the Shiny app (and future tests there).

So let's just wait for the SOP. In the meantime, I'm going to complete the NEWS file and CRAN comments to get ready for submission.