traitecoevo / plant

Trait-Driven Models of Ecology and Evolution :evergreen_tree:
https://traitecoevo.github.io/plant
53 stars 20 forks source link

Proposed fix to issue with integration of density #345

Closed Becca-90 closed 2 years ago

Becca-90 commented 2 years ago

Issue #344

aornugent commented 2 years ago

Thanks @Becca-90!

You can see above that your PR failed the automated tests by the little ❌ next your commit - which isn't a problem because it tells you what to fix!

Clicking through one of the R-CMD-check actions below and scrolling all the way to the bottom takes me to this error message.

The fix is simple: you need to add the dplyr:: namespace to rename.

You can run these tests locally, before pushing your changes with devtools::test() or the keyboard shortcut Ctrl + Shift + T.

I wasn't able to tell from just reading your code how these changes fixed the issue. Could you please describe your solution?

Becca-90 commented 2 years ago

Morning!

So the issue was in the last part of the integrate_across_size_distribution function. We previously had individuals calculate like this:

    dplyr::summarise(
      individuals = -trapezium(.data$height, .data$density),
      min_height = min(.data$height),
      dplyr::across(where(is.double), ~ -trapezium(height, density*.x)),
      .groups="drop"
    )

it was basically causing the trapezium integration to be done twice for individuals and giving some crazy output... so in the second call of trapezium (below) we just had to isolate everything that isn't already integrated or needing to be, to integrate across... I think that's essentially what was going on! We also renamed individuals as density integrated (a bit clearer?)

    dplyr::summarise(
      density_integrated = -plant:::trapezium(.data$height, .data$density), 
      min_height = min(.data$height),
      dplyr::across(where(is.double) & !c(density, density_integrated, min_height) , ~-plant:::trapezium(height, density * .x)), 
        .groups = "drop") %>% 
    rename(density = density_integrated)
codecov-commenter commented 2 years ago

Codecov Report

:exclamation: No coverage uploaded for pull request base (develop@27e391c). Click here to learn what that means. The diff coverage is n/a.

@@            Coverage Diff             @@
##             develop     #345   +/-   ##
==========================================
  Coverage           ?   79.64%           
==========================================
  Files              ?       97           
  Lines              ?     8896           
  Branches           ?        0           
==========================================
  Hits               ?     7085           
  Misses             ?     1811           
  Partials           ?        0           

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 27e391c...d866998. Read the comment docs.

aornugent commented 2 years ago

@dfalster - I'm happy to merge this without a dedicated test. A test can be submitted in a second PR. Thoughts?

dfalster commented 2 years ago

Hold off on the merge. I'll add a test tomorrow. The template code is already there