FoRTExperiment / fortedata

FoRTE project data
https://fortexperiment.github.io/fortedata/
Creative Commons Attribution 4.0 International
7 stars 11 forks source link

ESSD checks and fixes needed #56

Closed atkinsjeff closed 3 years ago

atkinsjeff commented 3 years ago

Necessary changes/updates needed for ESSD review. I have @'ed you to get input or check and see if you can help with this task:

Note from Jeff: The last part of this I can address in text so that part can go to me (@atkinsjeff ) but if someone wants to add code to change FAGRE to FAGR, etc. that would be cool. The SWD thing should be CWD for coarse woody i think. @stephpenn1

COMMENTS FROM REVIEWER ONE

If you see any more issues, please add. --jeff

bpbond commented 3 years ago

Should we embed measurement units in all column names? Note from Jeff, I want consistency, but could be unwieldy.

Definitely not; ugly, cumbersome, and often ambiguous. Metadata define the units for all columns.

@atkinsjeff Sorry just catching up with this. Please keep in mind that many of us get LOTS of GitHub email...it's not hard to miss something, like e.g. being tagged just once. I will work through items above as I can in next few days.

atkinsjeff commented 3 years ago

no worries @bpbond hope i didn't come across as pushy!

lisahaber commented 3 years ago
* fd_leaf_spectrometry: Reviewer comments: "Was this dataset exported as only the head of the data? Only 8 rows of data import using the R package when 7,155 are expected. There is an additional column “tree_id” that is not in the format of USDA PLANTS species codes and not defined under Table S6 in the SI."     @lisahaber @atkinsjef

"Tree_id" column in this data set is not supposed to be a species code (that is a separate column called "species"). The information currently in this column is also not actual tree ID for these individuals, however. That would come from a lookup table with the IDs which were appended in 2019.

atkinsjeff commented 3 years ago

@lisahaber I fixed the issue with the loss of rows, but what do how should we proceed to address the ID issue?

lisahaber commented 3 years ago

I can send you the correct IDs, 2018 leaf IDs mapped to 2019 (i.e. the permanent) tree IDs.

A related issue is this: For the subcanopy data (not currently in the fd_leaf_spectrometry() data set, but eventually will be included) there is no leaf ID, because I only sampled once per tree. For these canopy trees, though, I think we need another column ("leaf_id") because I measured 3 leaves per tree.

lisahaber commented 3 years ago

Ok @atkinsjeff here's the look-up table for correcting the tree_id column in the fd_leaf_spectrometry() data. Note that what I'm listing as "leaf_id" in this table is what we currently have as "tree_id" in fd_leaf_spectrometry(). You need BOTH the subplot and the leaf_id columns to match the trees to their correct tree_id.

lisahaber commented 3 years ago

Whoops...here it is. Let me know if you have questions or trouble. Canopy_tree_lookup_table.xlsx

ashiklom commented 3 years ago

fd_metadata(table = “fd_inventory”) returns the whole metadata tibble.

This should be fixed in #60. There's also a bit more cleanup in there.

atkinsjeff commented 3 years ago

Ok @atkinsjeff here's the look-up table for correcting the tree_id column in the fd_leaf_spectrometry() data. Note that what I'm listing as "leaf_id" in this table is what we currently have as "tree_id" in fd_leaf_spectrometry(). You need BOTH the subplot and the leaf_id columns to match the trees to their correct tree_id.

@lisahaber can we just change "tree_id" to "leaf_id" then? i am confused

lisahaber commented 3 years ago

@atkinsjeff unfortunately, no, because there are three leaves per tree. I am sorry this is so confusing. Did not think through naming carefully enough in 2018.

cmgough commented 3 years ago

@atkinsjeff, I'd favor a re-ordering of the vignettes, starting with "fortedata: Proposal Narrative", then "fortedata: Experimental Design and Treatment", with dataset vignettes following.

cmgough commented 3 years ago

Moving this thread to Git for others to see, regarding R2's data visualization comment. @atkinsjeff, @bpbond, I'm wondering whether a common template (or 2) for data/dataset visualization could/should be coded into the package for dataset-focused vignettes. Here's what I posted to Slack:

Does anyone have thoughts with respect to a useful template or format for standardized data visualization across datasets? Would it make sense to generate two figures for each dataset vignette? E.g., one describing data availability, see: https://data.neonscience.org/data-products/explore. This would seem to address R2’s criticism. And, maybe another displaying the treatment means (no stats) of a response parameter within the dataset – a teaser of sorts. This would serve more as a useful illustration for orienting outside users.

cmgough commented 3 years ago

@atkinsjeff, litter data vignette: Doesn't appear to conform to the same structure as other vignettes and is without methods. Remote sensing vignette methodological details differ among subsections and, unlike some of the other dataset vignettes, provide less detail on sampling distribution.

bpbond commented 3 years ago

Does anyone have thoughts with respect to a useful template or format for standardized data visualization across datasets?

First, I think having a general template is an excellent idea, as it makes life easier for both vignette creators and package users. (A few, like the proposal vignette, won't conform to this of course.)

I like Chris's starting thoughts above about what the template actually should be. Building on that a bit:

...or something like that?

Note: forest inventory vignette includes a static number ("There are 3165 observations in the dataset") that should be calculated from the data.

atkinsjeff commented 3 years ago

That seems reasonable. I put the static number in there because I didn't figure we were adding more trees :)

I will make it dynamic.

atkinsjeff commented 3 years ago

I did some work on the remote sensing vignette this morning: https://fortexperiment.github.io/fortedata/articles/fd_remote_sensing_vignette.html

There are some issues on the observations waffle plot after pkgdown, but outside of that everything works fine. Still needs some work but @cmgough is this closer to what you envisage?

bpbond commented 3 years ago

file size appears to an issue. our largest files are far and away vignette files. can these be compressed further or left out for CRAN? @bpbond @stephpenn1 @atkinsjeff

Well, why are they large? I assume because of graphics files. You should be able to experiment with reducing file sizes and/or resolutions to make things smaller.

bpbond commented 3 years ago

add code to change FAGRE to FAGR

Why would we do this in code? Why not just change the data files?

atkinsjeff commented 3 years ago

Agreed. It will be a balance act I think to thread the needle between lots of documentation and illustration and file size.

Jeff

Jeff Atkins, Ph.D Department of Biology Virginia Commonwealth University

On Mon, Dec 7, 2020, 07:10 Ben Bond-Lamberty notifications@github.com wrote:

file size appears to an issue. our largest files are far and away vignette files. can these be compressed further or left out for CRAN? @bpbond https://github.com/bpbond @stephpenn1 https://github.com/stephpenn1 @atkinsjeff https://github.com/atkinsjeff

Well, why are they large? I assume because of graphics files. You should be able to experiment with reducing file sizes and/or resolutions to make things smaller.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/FoRTExperiment/fortedata/issues/56#issuecomment-739878837, or unsubscribe https://github.com/notifications/unsubscribe-auth/AB7XVVOKH3YUGLOKLWBZQL3STTAZZANCNFSM4UHX3DVA .

atkinsjeff commented 3 years ago

Ultimately I did change the errors in data.

Jeff Atkins, Ph.D Department of Biology Virginia Commonwealth University

On Mon, Dec 7, 2020, 07:11 Ben Bond-Lamberty notifications@github.com wrote:

add code to change FAGRE to FAGR

Why would we do this in code? Why not just change the data files?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/FoRTExperiment/fortedata/issues/56#issuecomment-739879634, or unsubscribe https://github.com/notifications/unsubscribe-auth/AB7XVVNTTHHBHHARXP5MFNDSTTA67ANCNFSM4UHX3DVA .

cmgough commented 3 years ago

I did some work on the remote sensing vignette this morning: https://fortexperiment.github.io/fortedata/articles/fd_remote_sensing_vignette.html

There are some issues on the observations waffle plot after pkgdown, but outside of that everything works fine. Still needs some work but @cmgough is this closer to what you envisage?

Following Ben’s suggestion, which I like, the vignette lacks: 1) an intro/broad subject of the vignette needed to orient the user and, 2) importantly, linked references.

Some of the methods lack essential details on sampling intensity and location.

I’d also clearly label the “Methods description” section with a heading.

@atkinsjeff, are you building the figure templates (1—data availability & 2—treatment means) or is that assigned to someone else?

atkinsjeff commented 3 years ago

the figures are not currently showing up via pkgdown, but are in the in-package vignettes. I was going with simple boxplots for now. I am adding the references now. I was just trying to draft up what I could then. I will expand the intro and touch base with others about expanding theirs as well.

On Mon, Dec 7, 2020 at 7:55 AM Chris Gough notifications@github.com wrote:

I did some work on the remote sensing vignette this morning: https://fortexperiment.github.io/fortedata/articles/fd_remote_sensing_vignette.html

There are some issues on the observations waffle plot after pkgdown, but outside of that everything works fine. Still needs some work but @cmgough https://github.com/cmgough is this closer to what you envisage?

Following Ben’s suggestion, which I like, the vignette lacks: 1) an intro/broad subject of the vignette needed to orient the user and, 2) importantly, linked references.

Some of the methods lack essential details on sampling intensity and location.

I’d also clearly label the “Methods description” section with a heading.

@atkinsjeff https://github.com/atkinsjeff, are you building the figure templates (1—data availability & 2—treatment means) or is that assigned to someone else?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/FoRTExperiment/fortedata/issues/56#issuecomment-739900757, or unsubscribe https://github.com/notifications/unsubscribe-auth/AB7XVVLRQ6FM7FRVQ42R6KLSTTGDHANCNFSM4UHX3DVA .

-- Jeff Atkins, PhD Post-Doctoral Fellow Department of Biology Virginia Commonwealth University atkinsjeff.github.io he/his/him

lisahaber commented 3 years ago

Hey folks, catching up here -- I'm still happy to fix up the Ecophysiology vignette and include standard figures Chris mentioned. Probably best to wait for a working example to emulate, though, so I'll stay tuned for that. The methods and data description in this vignette seem thorough to me but happy to expand/modify if anyone thinks we should.

atkinsjeff commented 3 years ago

Working on this. Things are :/ in general so far.

Jeff Atkins, Ph.D Department of Biology Virginia Commonwealth University

On Mon, Dec 7, 2020, 09:24 Lisa T. Haber notifications@github.com wrote:

Hey folks, catching up here -- I'm still happy to fix up the Ecophysiology vignette and include standard figures Chris mentioned. Probably best to wait for a working example to emulate, though, so I'll stay tuned for that. The methods and data description in this vignette seem thorough to me but happy to expand/modify if anyone thinks we should.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/FoRTExperiment/fortedata/issues/56#issuecomment-739948506, or unsubscribe https://github.com/notifications/unsubscribe-auth/AB7XVVMWX7WO5SYPOF7WYCTSTTQH7ANCNFSM4UHX3DVA .

atkinsjeff commented 3 years ago

For the life of me I cannot figure out why some vignettes build plots no issue and others simply do not.

kdorheim commented 3 years ago

Are the figures just not rendering or is there an error message associated with it?

atkinsjeff commented 3 years ago

I get nothing: https://fortexperiment.github.io/fortedata/articles/fd_remote_sensing_vignette.html

But using basically the same code: https://fortexperiment.github.io/fortedata/articles/fd_litter_vignette.html

The litter vignette does fine. I guess I can cut the images and see if that does anything?

atkinsjeff commented 3 years ago

however the more i look at this, it seems i may not being doing the correct build procedure for pkgdown in order to update

atkinsjeff commented 3 years ago

I am closing this issue. the two remaining issues here are long term.