assorted typos and other minor issues about r3pg HOT 6 CLOSED

Thanks @twest820 for the outstanding code review and pointing potentially some issues. Some of them shall be addressed back to original code of Landsberg and Waring some of them indeed need to be cross-checked with 3-PG mix.

Would be great if there will be a volunteer who can implement those suggestions and corrections.

A quick response from my side:

md_3PG.f95

Based on the excel version downloaded here https://sites.google.com/site/davidforresterssite/home/projects/3PGmix, r3PG have the same implementation and coefficients:

If CanopyVolumefraction(layer) < 0.01 Then
                    lambdaH(speciescounter) = 0.8285 + ((1.09498 - 0.781928 * kLSweightedave(layer)) * 0.1 ^ (0.01)) - 0.6714096 * 0.1 ^ (0.01)
                    Else
                    lambdaH(speciescounter) = 0.8285 + ((1.09498 - 0.781928 * kLSweightedave(layer)) * 0.1 ^ (CanopyVolumefraction(layer))) - 0.6714096 * 0.1 ^ (CanopyVolumefraction(layer))
                    End If

md_decl_const.f95

Yes, correct. There is a room for improvement, as you also mentioned in #68

i_write_out.h

Fortran output is a four dimensional array. Fortran is not communicating the names back to R and therefore the names are assigned in R transf.out . Therefore, there is no real need to declare the unused variables in Fortran.

prepare_climate.R

good suggestion.

prepare_sizeDist.R

good suggestion

r3PG_VBA_compare.R

There are automatic tests run under the testthat, but agree would be nice to have better automatic testing with excel.

Much room for improvement.

from r3pg.

Tod can you please write the new comment instead of extending the initial one, this would be easier to track for me. Thanks

i_write_output.h

! Production ---------------
output(ii,:,6,2) = npp_f(:)

This is because it is assigned NPP values for further calculation

! Mortality ---------------
output(ii,:,8,1) = biom_tree_max(:)

For me seems to be correct and is a time variant variable

library(r3PG)
out_3PG <- run_3PG(
  site        = d_site, 
  species     = d_species, 
  climate     = d_climate, 
  thinning    = d_thinning,
  parameters  = d_parameters, 
  size_dist   = d_sizeDist,
  settings    = list(light_model = 2, transp_model = 2, phys_model = 2, 
                     height_model = 1, correct_bias = 0, calculate_d13c = 0),
  check_input = TRUE, df_out = TRUE)


library(dplyr)
library(ggplot2)

sel_var <- c('biom_tree_max')

out_3PG %>%
  filter( variable %in% sel_var ) %>%
  ggplot( aes(date, value, color = species) ) +
  geom_line() +
  facet_wrap(~variable, scales = 'free') +
  theme_classic()

Third, this probably isn't really an i_write_out.h issue

answered in previous comment #69 (comment)

prepare_climate.R

There are a couple issues with this.

Yes, thanks for this. The issue was that I was checkin for NA in all columns instead of the subset (other assumptions which you suggested were already included before).

prepare_input.R

Thanks, DONE

prepare_sizeDist.R

Thanks, DONE

from r3pg.

s_sizeDist_correct

s_sizeDist_correct() declares but does not initialize CVdbhDistribution, CVwsDistribution, wsWeibullScale, wsWeibullShape, and wsWeibullLocation. It also does not zero them in these two branches:

For me everything works well, can you please provide reproducible example:

library(r3PG)
out_3PG <- run_3PG(
  site        = d_site, 
  species     = d_species, 
  climate     = d_climate, 
  thinning    = d_thinning,
  parameters  = d_parameters, 
  size_dist   = d_sizeDist,
  settings    = list(light_model = 2, transp_model = 2, phys_model = 2, 
                     height_model = 1, correct_bias = 0, calculate_d13c = 0),
  check_input = TRUE, df_out = TRUE)


library(dplyr)
library(ggplot2)

sel_var <- c('DrelBiaspFS', 'DrelBiasBasArea')

out_3PG %>%
  filter( variable %in% sel_var ) %>%
  ggplot( aes(date, value, color = species) ) +
  geom_line() +
  facet_wrap(~variable, scales = 'free') +
  theme_classic()

from r3pg.

md_3PG.f95

assume age(, :) and the total basal_area are greater than zero. This is not necessarily the case, may contribute to #52,

Can you please provide a reproducible example as discussed in #52

from r3pg.

As the necessary state management code simply isn't present I think md_3PG.f95 itself is probably adequate as a repex (I added those lines in my branch the same day as noting the issue here, which reasonably also qualifies). In my view, at least, the issue really is that timestep state management isn't object oriented. While I've moved state into its own class (and also fixed another occurrence of this particular issue with t_n) I haven't refactored into a state.ClearSizeDist() kind of member function. I probably should.

If the package tests did repeated runs on the r_vba_compare examples in principle they could pick up this particular issue—that's where my unit tests found it—but I suspect the call pattern through R introduces additional zeroing which masks the Fortran-level omissions. Since it's most likely a latent defect that's why it's noted in this list of minor miscellany rather than broken out as its own issue.

from r3pg.

Hi Tom,
Yes, r3PG eventually diverged from eh excel 3PG mix due to implementations and further development. Therefore, I'm not sure if the further comparison with the excel version will be valuable. We also, during the previous tests, noted that some calculations (e.g calculation average temperature from minimum and maximum) will give a different results between R/Fortran and Excel (on the >6th digit) this will eventually result also in diverge calculations.

from r3pg.