kaizadp / nmrrr

Binning and visualizing NMR spectra from environmental samples
Other
3 stars 1 forks source link

Status #25

Closed bpbond closed 1 year ago

bpbond commented 2 years ago

I won't have much time this coming week but @kaizadp please test out the package, in particular reading spectra and peaks files, and assigning bins. Feedback welcome.

kaizadp commented 2 years ago

Thanks! Working through these edits today.

bpbond commented 2 years ago

Thanks for merging PRs 30-34 @kaizadp . At this point, the package is pretty solid I think 🎉

Remaining to-do items from my POV:

Anything else?

kaizadp commented 2 years ago

Transfer ownership to https://github.com/COMPASS-DOE ?

Possibly? This is not exclusively a COMPASS package (COMPASS/TES/RC contributions), so not sure if it's better to place it under COMPASS, or leave it as your/my repo. Let's discuss.

I would prefer to remove dplyr as an import; not much code actually uses it, and it's a heavy dependency. But defer to you @kaizadp

Sure! I assumed we needed to import every package that is being used in our functions, so I added it in there. But yes, let's delete if it's not needed and making the package unwieldy.

Let's talk about the data in inst/extdata - at 50+ MB, it's triggering our only NOTE (which will hold up approval on CRAN)

Just looked a couple of the files. kfp_hysteresis/spectra_mnova/ files are so big because there are a lot of "crap" data (< 0 or > 10 ppm), I deleted those, and brought it down from 1.3 MB to 740 KB! I'll do this for all the files and see how much that helps the overall size.

bpbond commented 2 years ago

Oh, if not COMPASS, fine. I just assumed. Thanks!

bpbond commented 2 years ago

With e2554ea0efb364b5ccd1214e1bc2e50052a05d36 I think we're in good shape. The remaining unchecked items above are mostly on you @kaizadp :

Happy to consult about any of these! But for now, I'm moving this off my to-do list 😄

kaizadp commented 2 years ago

Thanks! Yep, working on these items, and I’ll tag you in Issues/PRs.

bpbond commented 2 years ago

Probably obvious, but: any datasets in inst/extdata should be used — either in @examples or the vignette(s).

kaizadp commented 1 year ago
  • [x] Let's talk about the data in inst/extdata - at 50+ MB, it's triggering our only NOTE (which will hold up approval on CRAN)

Finally done, PR #44

bpbond commented 1 year ago

@kaizadp Maybe we can touch base in the new year about this?

kaizadp commented 1 year ago

Yes! Sorry, it's been a crazy few weeks/months. I'll work on the vignettes soon, and we can touch base in January. Thanks!