search

FoRTExperiment / fortedata

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

Aboveground wood data updates and vignette fixes #83

Closed mgrigri1 closed 3 years ago

mgrigri1 commented 3 years ago

Per @atkinsjeff 's request I added all the aboveground functions to the fd_inventory script (and removed all individual scripts). I think there's a case to be made for a separate fd_aboveground script that houses all the aboveground functions, but understand wanting to limit the number of scripts in the package. A few other small changes getting the aboveground vignette up to speed. Next will be adding a figure to the vignette

mgrigri1 commented 3 years ago

@atkinsjeff seems the pull request passes all but one check. A quick look and it seems to be something to do with dependencies in the aboveground vignette .rmd. Lmk how I can help move this along

bpbond commented 3 years ago

@mgrigri1 Hang on I will try pushing a fix.

mgrigri1 commented 3 years ago

oh sweet, thanks!

bpbond commented 3 years ago

It looks like udunits2 just isn't yet available for the development version of R. I have reverted my commit and think we can ignore this particular failure.

codecov-commenter commented 3 years ago

Codecov Report

Merging #83 (6f7f497) into master (c62bca2) will increase coverage by 1.88%. The diff coverage is 100.00%.

:exclamation: Current head 6f7f497 differs from pull request most recent head 883b9c3. Consider uploading reports for the commit 883b9c3 to get more accurate results Impacted file tree graph

@@            Coverage Diff             @@
##           master      #83      +/-   ##
==========================================
+ Coverage   77.48%   79.37%   +1.88%     
==========================================
  Files          14       11       -3     
  Lines         382      417      +35     
==========================================
+ Hits          296      331      +35     
  Misses         86       86
Impacted Files Coverage Δ
R/fd_inventory.R 81.33% <100.00%> (+22.50%) :arrow_up:
R/fd_observations.R 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update c62bca2...883b9c3. Read the comment docs.

mgrigri1 commented 3 years ago

@bpbond okay, sounds good to me. Yeah I don't even what udunits2 is. Is my system using it in behind the scenes or soemthing?

bpbond commented 3 years ago

It's a piece of software, and here R package, for unit conversion and physical constants. Here it's used in a single place, to confirm that all the metadata units are correct (well, parseable anyway). I'm not sure this is worth a full extra dependency, given that udunits2 doesn't seem to be updated very frequently...we might consider removing this test entirely.

I suspect this was introduced by @ashiklom . Alexey, any thoughts?

ashiklom commented 3 years ago

Personally, I think it's a useful test -- ensuring machine-parseable units can save all kinds of headaches.

If the failure is only on the dev version of R, my top recommendation would be to just make tests on the dev version of R optional to the CI -- that way, you have some warning about forthcoming failures, but they don't hold up perfectly good PRs in stable R versions. There should be a setting in the repo. Another option is to tweak the test to skip if udunits2 is not installed (and then remove it from the list of dependencies). Or you could just ditch the test altogether. I promise I won't be offended if you delete that code :)

atkinsjeff commented 3 years ago

yeah i am totally fine removing that. I think there was some hang up early on where it was used in some conversion. I am sure we can get an easy workaround.

On Wed, Apr 28, 2021 at 2:43 PM Ben Bond-Lamberty @.***> wrote:

It's a piece of software https://www.unidata.ucar.edu/software/udunits/udunits-current/udunits2.html, and here R package, for unit conversion and physical constants. Here it's used in a single place, to confirm that all the metadata units are correct (well, parseable anyway). I'm not sure this is worth a full extra dependency, given that udunits2 doesn't seem to be updated very frequently...we might consider removing this test entirely.

I suspect this was introduced by @ashiklom https://github.com/ashiklom . Alexey, any thoughts?

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

-- Jeff Atkins

Post-Doctoral Scholar Department of Biology Virginia Commonwealth University

Visiting Scholar Department of Environmental Sciences University of Virginia

atkinsjeff commented 3 years ago

adding a step to skip seems reasonable. and i know you will be SUPER offended

On Wed, Apr 28, 2021 at 2:52 PM Jeff Atkins @.***> wrote:

yeah i am totally fine removing that. I think there was some hang up early on where it was used in some conversion. I am sure we can get an easy workaround.

On Wed, Apr 28, 2021 at 2:43 PM Ben Bond-Lamberty < @.***> wrote:

It's a piece of software https://www.unidata.ucar.edu/software/udunits/udunits-current/udunits2.html, and here R package, for unit conversion and physical constants. Here it's used in a single place, to confirm that all the metadata units are correct (well, parseable anyway). I'm not sure this is worth a full extra dependency, given that udunits2 doesn't seem to be updated very frequently...we might consider removing this test entirely.

I suspect this was introduced by @ashiklom https://github.com/ashiklom . Alexey, any thoughts?

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

-- Jeff Atkins

Post-Doctoral Scholar Department of Biology Virginia Commonwealth University

Visiting Scholar Department of Environmental Sciences University of Virginia

-- Jeff Atkins

Post-Doctoral Scholar Department of Biology Virginia Commonwealth University

Visiting Scholar Department of Environmental Sciences University of Virginia

bpbond commented 3 years ago

It's so embarrassing trying to debug GitHub Actions. Everyone sees your mistakes :)

bpbond commented 3 years ago

OK, we now skip tests and vignette building on r-devel, which is one of the seven configurations we build. That seems OK.

I don't know what the heck is happening with the segmentation fault in the macOS-latest (3.6). Weird.

ashiklom commented 3 years ago

@bpbond This works too, but for the record, I actually meant do run the tests on r-devel (and MacOS R 3.6, if you want), but don't make them a required pre-condition for the build to mark as passing. For example, see the checks in https://github.com/PecanProject/pecan/pull/2780 -- note how only a subset of the checks are marked as "Required". IIRC, the PR will get a ✅ even if some of the non-required builds are failing (not the case with that PR).

That is configured from the repository settings: Settings --> Branches --> Branch Protection Rules --> Edit (on a specific branch) / or "Add rule" if none are configured --> "Require status checks before merging" --> Uncheck the ones that aren't required.

bpbond commented 3 years ago

@ashiklom Ah I misunderstood what you were proposing; thanks for clarifying. Well, it was a good exercise to modify the GitHub Actions file to have tests skipped for r-devel and if there are no objections I'll just leave it as is.

atkinsjeff commented 3 years ago

are we good to merge? looks good to me

On Thu, Apr 29, 2021 at 5:29 AM Ben Bond-Lamberty @.***> wrote:

@ashiklom https://github.com/ashiklom Ah I misunderstood what you were proposing; thanks for clarifying. Well, it was a good exercise to modify the GitHub Actions file to have tests skipped for r-devel and if there are no objections I'll just leave it as is.

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

-- Jeff Atkins

Post-Doctoral Scholar Department of Biology Virginia Commonwealth University

Visiting Scholar Department of Environmental Sciences University of Virginia

bpbond commented 3 years ago

Yep, go ahead

mgrigri1 commented 3 years ago

Thanks for jumping on this everyone! And sorry if it was a bit sloppy. Hope to make this smoother moving forward

atkinsjeff commented 3 years ago

merged.

mgrigri1 commented 3 years ago

@atkinsjeff hmmm, I don't think this PR ever merged, it was just closed...ideas?

bpbond commented 3 years ago

Wow. No, it was not.