torv72 / torv-reports-v4

1 stars 0 forks source link

New Error "object 'run_om_accumulation' not found #33

Open torv72 opened 1 week ago

torv72 commented 1 week ago

I was running a test report for an upcoming client meeting with Snowmass Club and I am receiving an error I have not seen before.

Here is the parameters for the report:

generate_report(.site_name = "Snowmass Club", .zip_code = 81615, .date_sample_submitted = "2023-09-13", .start_date = "2018-01-01", .om_seasons = "Spring", .warm_or_cool = "cool", .acid_extract = "Mehlich", .include_results_interpretation = FALSE, .include_sand_fraction = TRUE, .draw_beeswarm = "Yes", .output = c("pdf", "html"))

Then here is the error I am getting once I execute the code:

processing file: report.Rmd |......... | 9% [setup]

processing file: templates/setup.Rmd

🗂️ I found the following soil types in the database: GREEN, TEE, FAIRWAY, APPROACH. ❗ I didn't find any water types in the database. 🗂️ I found the following OM types in the database: GREEN, TEE, FAIRWAY.

âť“ Does that seem correct, or do you want to exit to investigate the database before running the rest of the report? Type c for correct and hit ENTER to proceed, or hit any other key followed by ENTER to exit now.c Error: ! object 'run_om_accumulation' not found Backtrace:

  1. base::source(here("aux-scripts", "om-calculations.R"))
  2. base::eval(ei, envir)
  3. base::eval(ei, envir) Called from: signal_abort(cnd) Browse[1]>

Quitting from lines 477-491 [prepare-element-tables-and-auto-comments] (templates/setup.Rmd)

Quitting from lines 25-28 [setup] (report.Rmd)

z3tt commented 1 week ago

Investigating this further, this seems to be a special case when there is no or only one (?) entry for OM, that's why we've never run into this before. Fixed it.

This example now features APPROACH which is not listed among the common soil types. Should we add a default threshold for this soil type? (Currently, we provide defaults for GREEN, TEE, FAIRWAY, and ROUGH.)

EDIT: I am currently running into another error and am investigating that. Please do not run the current code yet.

@torv72 The new error is related to the different handling of seasons again. If I recall correctly, the season filter should be applied for all OM data correct? This is how it is currently done except for the setup step were it searches for the different types. Pretty sure this is also the reason for the issue #4 (APPROACH OM being shown).

We are running into another issue with the current approach when filtering the data to extract the types. For some reason, the workflow is to generate a "filtered database" for that. This filtering is based on .date_sample_submitted which in your case here returns data for September 2023 only. So far, so good.

But as .om_seasons is specified as "Spring", we run into issues with some types being returned that are not measured during that season and vice versa. Is this a valid setup that you are filtering for a season that is not in line with the date? What is the logic then, using soil and water testing from that date but OM data for the season specified? I am hesitating to simply change that as I can't foresee how it will cascade into other parts of the analysis and report. If that is a valid use case, I guess that discussing the logic in a short meeting makes more sense.

torv72 commented 1 week ago

We could add a default threshold for the APPROACH. We can use the same defaults that we use for FAIRWAY. I rarely test APPROACHs but from time to time I do.

I see where you are going with this regarding the seasonal filter and I think it is making sense that is what is happening at some level.

So when I create a report for say just soil and water. I enter the sampling date which is usually the same date since I took the samples at the same time. The report then produces the Executive Summary which is based only the results from that sample date. It does not include any past history, etc. The Executive Summary ONLY INCLUDES comments or information that pertains to that sample date's samples. If for example, I did not take a water test, it would not show up in the Executive Summary. If I only took soil samples from GREEN and nothing else, the only thing that shows up in Executive Summary is GREEN. The graphics and the rest of the report would also only pertain to GREEN. I think the report is doing this right now with soil and water. I don't think we have an issue there. Only organic matter.

The graphics though show the time lines and are based on the _start_date that I specify in the parameters and go to the present sample date. Again, if for example, water was not sampled, it would not appear in the report at all.

So with all of that being said, what I have noticed is that it seems like the seasonal filter works for the graphics portion of the report for Organic Matter. When I call for Spring, only Spring dates show in the graphics. Same for Autumn and same for "all". However as you pointed out I think it is somehow affecting what is being put into the Executive Summary.

The way I envision it to run for Organic Matter is like the Soil and Water already do. If I conducted organic matter sampling it should be included in the report and ONLY what I conducted. It should also do like it did correctly in the Maroon Creek sample, look back at the last time I did it in the same season and show the difference. If it was the first time I did it in that season or it is a first time visit with the client it obviously would not show the difference.

In Maroon Creek's 2024-06-06 it should have only one bullet point under Organic Matter and that is a reference to 0-2 cm as that is all I tested that day. This is a special case in all fairness as I never just do 0-2 cm. Normally I do all three depths.

For the graphics I would have expected "Spring" and just the 0-2 cm depth showing up.

I realize as I am typing, this is long. Hopefully it gives some insight but I'm happy to meet if it makes sense.

My head is hurting:)

torv72 commented 1 week ago

If you see a better way and or easier way to do it and present the data, I'm open to your thoughts and expertise. I'm always open to new ideas.

z3tt commented 1 week ago

I agree with you and that all makes sense. And actually it is already implemented like that (except for the special case here).

However, the original question still remains: Does it make sense to show only Spring measurements for OM when you are creating an Autumn report?

My naive answer would be "no" and thus I'd implement a check here. Otherwise, we have this weird mixture of OM types for the latest sampling date (e.g. Autumn testing) and a potentially different list for the season you specify (e.g. Spring). In that case, we need to extract the OM types twice, once for the sampling date (which goes into the exec summary, Autumn in my example) and once for the trendline charts (as specified in .om_seasons, Spring in my example). This is sounding rather strange to me: a summary of Autumn OM testing but visualizations showing Spring data.

torv72 commented 1 week ago

I’m sorry.  I 100% agree with you. No. It does not make sense to mix the seasons.   Spring with Spring and Autumn with Autumn. We do not want to show Autumn with Spring or vice versa unless I use “all”. Sent from my iPhoneOn Sep 5, 2024, at 9:24 AM, Cédric Scherer @.***> wrote: I agree with you and that all makes sense. And actually it is already implemented like that (except for the special case here). However, the original question still remains: Does it make sense to show only Spring measurements for OM when you are creating an Autumn report? My naive answer would be no and thus I'd implement a check here. Otherwise, we have this weird mixture of OM types for the latest sampling date (e.g. Autumn testing) and a potentially different list for the season you specify (e.g. Spring).

—Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you were mentioned.Message ID: @.***>

z3tt commented 1 week ago

Okay, so if I understand you correctly, the season would then be specified as `"Autumn" here?

generate_report(
  .site_name = "Snowmass Club",
  .zip_code = 81615,
  .date_sample_submitted = "2023-09-13",
  .start_date = "2018-01-01",
  .om_seasons = "Spring",
  .warm_or_cool = "cool",
  .acid_extract = "Mehlich",
  .include_results_interpretation = FALSE,
  .include_sand_fraction = TRUE,
  .draw_beeswarm = "Yes",
  .output = c("pdf", "html")
)

With this command, the report renders without errors now as the types are consistent for both, the sampling date and the seasons used for OM.

If you like, we can add a check to verify that the season of .date_sample_submitted matches .om_seasons in case it's not "all".

APPROACH now uses 0.7 as the default Grass Maximum N/month lb/1000 sq ft values for the deficit tables.

Note: I've updated the codes so that the inputs for .output can be any combination of lowercase and uppercase letters. Now, specifying e.g. "HTML" would work as well.

torv72 commented 1 week ago

Yes.  It should be Autumn.  I see where you are going with this. Maybe adding a check to verify season as you suggested is in order.  I’m not getting any younger so I might forget🤣 Ugh…Sent from my iPhoneOn Sep 6, 2024, at 1:46 AM, Cédric Scherer @.***> wrote: Okay, so if I understand you correctly, the season would then be specified as `"Autumn" here? generate_report( .site_name = "Snowmass Club", .zip_code = 81615, .date_sample_submitted = "2023-09-13", .start_date = "2018-01-01", .om_seasons = "Spring", .warm_or_cool = "cool", .acid_extract = "Mehlich", .include_results_interpretation = FALSE, .include_sand_fraction = TRUE, .draw_beeswarm = "Yes", .output = c("pdf", "html") )

With this command, the report renders without errors now as the types are consistent for both, the sampling date and the seasons used for OM. If you like, we can add a check to verify that the season of .date_sample_submitted matches .om_seasons in case it's not "all".

—Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you were mentioned.Message ID: @.***>