search

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

separating app and package vignettes. #135

Closed wincowgerDEV closed 12 months ago

wincowgerDEV commented 12 months ago

Working on finalizing the vignettes.

wincowgerDEV commented 12 months ago

@zsteinmetz will pick back up tomorrow. Got the app and package sop separated but encountered more bugs than I thought I would today, good thing we are making the sop, really puts the workflows to the test. sop.rmd is now for the package and app.rmd is for the app. I haven't done anything more than copy the previous sop to the app.rmd yet but the package sop is coming together. Currently at the match step.

zsteinmetz commented 12 months ago

Alright, I'll jump in as soon as you're ready.

wincowgerDEV commented 12 months ago

@zsteinmetz, a couple of other last minute ideas that we can either do now or push until late but wanted to see what you thought.

  1. I was thinking we could use check_OpenSpecy() at the end of every function that outputs an OpenSpecy object as a default option that users can choose to opt out of. This will trigger all of our tests if the objects we are creating aren't up to specs. It also lets us control stuff across all the functions and tests with a single function.
  2. Was also thinking of continuing support for a single vector string of absorbance intensities in the functions where it makes sense. We could use x.default() to write the vector function, then call it from x.OpenSpecy(). I did this for make_rel() in this PR. I guess the risk of doing it that way is that if people send non-OpenSpecy, non-vector things they may get weird errors. Let me know what you think. Perhaps we make a x.vector() option for these cases and update the default message to include that the function accepts vectors in addition to OpenSpecy's?
zsteinmetz commented 12 months ago

Hey @wincowgerDEV, thanks for sharing your thoughts.

1. I was thinking we could use check_OpenSpecy() at the end of every function that outputs an OpenSpecy object as a default.

Wouldn't we've done a bad job if those checks fail? I mean, we've a number of checks in place already for the input data (mostly by only allowing OpenSpecy objects); so as long as we didn't do a mistake, the output should be an OpenSpecy object as well. Adding extra checks by default might bloat the functions a bit too much. What we could do though is adding some more check_OpenSpecy() calls to our testthat routines. What do you think?

2. Was also thinking of continuing support for a single vector string of absorbance intensities in the functions where it makes sense.

Also a bit reluctant here: we decided for OpenSpecy objects because they are less error-prone and easier to maintain. Adding more flexibility/variety for data input would counteract this. Having vector support for some functions and not for others would add extra confusion for the user, I guess. True, we have some remaining functions with vector support but I always considered them helpers for the real OpenSpecy functions. Maybe I just don't see the benefit yet, sorry :see_no_evil:

wincowgerDEV commented 12 months ago

Wouldn't we've done a bad job if those checks fail? I mean, we've a number of checks in place already for the input data (mostly by only allowing OpenSpecy objects); so as long as we didn't do a mistake, the output should be an OpenSpecy object as well. Adding extra checks by default might bloat the functions a bit too much. What we could do though is adding some more check_OpenSpecy() calls to our testthat routines. What do you think?

Yeah definitely! Alright I'll add the checks to the test that for now and if users find some exceptions we will build tests for them.

Also a bit reluctant here: we decided for OpenSpecy objects because they are less error-prone and easier to maintain. Adding more flexibility/variety for data input would counteract this. Having vector support for some functions and not for others would add extra confusion for the user, I guess. True, we have some remaining functions with vector support but I always considered them helpers for the real OpenSpecy functions. Maybe I just don't see the benefit yet, sorry 🙈

No worries, we can see how users try to use the functions and if it seems like a lot of people are trying to custom build vector routines (perhaps for efficiency with certain flows that we aren't currently expecting) then we use the more efficient routines or build a vector exception for those functions.

wincowgerDEV commented 12 months ago

@zsteinmetz, I feel good about where we are at with the vignettes now. Still some tidying up to do with the particle analysis functions to streamline the workflow and probably need to put in some more work on the app but I think we can share with our collaborators and get their help to finish it up. Let me know what you think.

wincowgerDEV commented 12 months ago

resolved conflict

wincowgerDEV commented 12 months ago

@zsteinmetz another thought, are you comfortable with us being listed as co-creators and maintainers of OpenSpecy? I feel like your level of effort on this project warrants it. I think originally we just put me down because I started it, but honestly without your work we wouldn't be anywhere near here.

zsteinmetz commented 12 months ago

No worries, we can see how users try to use the functions and if it seems like a lot of people are trying to custom build vector routines (perhaps for efficiency with certain flows that we aren't currently expecting) then we use the more efficient routines or build a vector exception for those functions.

What about writing as.data.table and as.data.frame methods for OpenSpecy objects? Then, all operations can be performed on OpenSpecy objects and converted in the end (or even in between) if needed.

zsteinmetz commented 12 months ago

Still some tidying up to do with the particle analysis functions to streamline the workflow and probably need to put in some more work on the app but I think we can share with our collaborators and get their help to finish it up. Let me know what you think.

Sounds good! I'm just having look at your latest changes. Should we prepare a beta release (GitHub only) afterwards or would you like to have it on CRAN asap?

zsteinmetz commented 12 months ago

another thought, are you comfortable with us being listed as co-creators and maintainers of OpenSpecy? I feel like your level of effort on this project warrants it. I think originally we just put me down because I started it, but honestly without your work we wouldn't be anywhere near here.

CRAN want a single maintainer only. We can keep it as is, no worries.

wincowgerDEV commented 12 months ago

What about writing as.data.table and as.data.frame methods for OpenSpecy objects? Then, all operations can be performed on OpenSpecy objects and converted in the end (or even in between) if needed.

Love the idea!

wincowgerDEV commented 12 months ago

Sounds good! I'm just having look at your latest changes. Should we prepare a beta release (GitHub only) afterwards or would you like to have it on CRAN asap?

I think I want to have it on CRAN because that way we get input from a broader group of folks. Also like last time I'm sure CRAN is going to have some new rules to throw at us and I don't want to wait till the end to have to incorporate their feedback. I think it's stable and ready to send there. It will also allow me to deploy the updated app to the openanalysis.org/OpenSpecy site more easily.

zsteinmetz commented 12 months ago

Alright. I prepare everything ..