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

separating app and package vignettes. #135

Merged
merged 37 commits into from
Sep 1, 2023
Merged

separating app and package vignettes. #135

merged 37 commits into from
Sep 1, 2023

Conversation

wincowgerDEV
Copy link
Owner

@wincowgerDEV wincowgerDEV commented Aug 30, 2023

Working on finalizing the vignettes.

  • separate the app and package vignettes
  • check that the online packagedown docs look good

@wincowgerDEV
Copy link
Owner Author

@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
Copy link
Collaborator

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

@wincowgerDEV
Copy link
Owner Author

@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
Copy link
Collaborator

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 🙈

@wincowgerDEV
Copy link
Owner Author

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
Copy link
Owner Author

@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
Copy link
Owner Author

resolved conflict

@wincowgerDEV
Copy link
Owner Author

@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
Copy link
Collaborator

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
Copy link
Collaborator

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
Copy link
Collaborator

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
Copy link
Owner Author

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
Copy link
Owner Author

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
Copy link
Collaborator

Alright. I prepare everything ..

vignettes/sop.Rmd Outdated Show resolved Hide resolved
@zsteinmetz zsteinmetz merged commit 3efca61 into main Sep 1, 2023
7 checks passed
@zsteinmetz zsteinmetz deleted the vignettes branch September 1, 2023 19:45
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.

2 participants