USFS-PNW / Fia-Biosum-Manager

User interface and main code repository for Biosum
http://biosum.info/
Other
3 stars 3 forks source link

OPTIMIZER: Calculation for USEBIOMASS_YN is incorrect #352

Open lbross opened 2 months ago

lbross commented 2 months ago

The costs are accounting for discounting (future costs are scaled down in today’s terms) but the future revenues are not being scaled down (no escalator term on the scenario_tree_species_diam_dollar_values.CHIP_VALUE but there needs to be (the chip revenue one). Lesley circulated an updated TREATMENT OPTIMIZER TABLE DEFINITIONS.docx for review.

Drue's code sample indicated that the wrong escalator is being used in the calculation. EnergyWoodRevenueCycle2Escalator should be OperatingCostsCycle2. Same for cycle 3-4. This needs to be fixed.

@druepdx will supply a test optimizer_results.db for the CWI project when these fixes are complete.

druepdx commented 2 months ago

I've made those changes to where usebiomass_yn is being set. Here is an example of one of the SQL statements:

UPDATE econ_by_rx_cycle_work_table SET usebiomass_yn = CASE WHEN CHIP_VALUE EnergyWoodRevenueCycle2Escalator < psite_accessible_work_table.CHIP_HAUL_COST_DPGT OperatingCostsCycle2Escalator THEN 'N' ELSE 'Y' END FROM psite_accessible_work_table WHERE econ_by_rx_cycle_work_table.biosum_cond_id = psite_accessible_work_table.biosum_cond_id AND usebiomass_yn = 'Y' AND rxcycle = '2'

I uploaded a test optimizer_results.db for scenario6 to Box in the Projects/cwi folder.

lbross commented 2 months ago

Thank you @druepdx! Can you also look into @jsfried question about the net revenue filter? He needs to know what value this filter is applied against. And are the components of that value adjusted by the escalators?

druepdx commented 2 months ago

Hoping the following is what you're looking for. Here's an example of where the net revenue filter is being set:

UPDATE all_cycles_effective AS e SET net_revenue_1 = CASE WHEN p.MAX_NR_DPA IS NOT NULL THEN p.MAX_NR_DPA ELSE 0 END FROM ECON_BY_RX_UTILIZED_SUM AS p WHERE e.biosum_cond_id = p.biosum_cond_id AND e.rxpackage = p.rxpackage

lbross commented 2 months ago

We need to dig a little deeper. Here is a link to the documentation when we added the weighted variables. See p. 13. This is when we also changed how the revenue filter is applied. If we look at the query for how affordable_YN is set. We should be able to back into what values we are checking against the filter ...

druepdx commented 2 months ago

The affordable_yn filter is set based on the settings supplied by the analyst in the Optimization Settings screen (see screenshot). The query is:

UPDATE all_cycles_optimization SET affordable_YN = CASE WHEN [DPA condition from Optimization Settings] THEN 'Y' ELSE 'N' END Screenshot 2024-09-06 135047

lbross commented 2 months ago

OK. So let's take an example. Say the filter is net_revenue_1 > 0. It looks like BioSum adds a column to the optimization table called net_revenue_1 that has the value that is evaluated against the filter. Where does the value for net_revenue_1 come from? And have the escalators been applied to it?

druepdx commented 2 months ago

It's definitely pretty intricate and kind of hard to follow, but it looks like the net_revenue_1 value comes from the MAX_NR_DPA value in the ECON_BY_RX_UTILIZED_SUM table. I haven't found anywhere where the escalators have been applied to it.

jsfried commented 2 months ago

Doesn't MAX_NR_DPA already have the escalators built into its calculation?

druepdx commented 2 months ago

This is the SQL statement that sets MAX_NR_DPA:

UPDATE econ_by_rx_cycle_work_table SET HAUL_COSTS_DPA = CASE WHEN usebiomass_yn = 'N' THEN MERCH_HAUL_COST_DPA ELSE MERCH_HAUL_COST_DPA + CHIP_HAUL_COST_DPA END, MAX_NR_DPA = CASE WHEN usebiomass_yn = 'Y' THEN MERCH_CHIP_NR_DPA ELSE MERCH_NR_DPA END

lbross commented 2 months ago

We need to work back further than this. net_revenue_1 value does come from the MAX_NR_DPA value in the ECON_BY_RX_UTILIZED_SUM table. The ECON_BY_RX_UTILIZED_SUM is populated with values from the econ_by_rx_cycle_work_table. I am working through this by reading the sql statements in the runlog.txt log file for the scenario that I run. Unfortunately the statement that inserts rows into the econ_by_rx_cycle_work_table is missing from the log so we can't see where the revenue calculations originally come from. It looks like this logging statement got lost during the translation to SQLite. @druepdx, can you add a log statement around line 10446 in uc_optimizer_scenario_run.cs and publish the sql to this issue? We need to log the 'insert' statement. It wouldn't be a bad idea to log the 'create table' statement just up from there too.

druepdx commented 2 months ago

I added those log statements. I'm also working on tracing back where exactly MAX_NR_DPA is coming from.

druepdx commented 2 months ago

I tracked the MAX_NR_DPA back to MERCH_VAL_DPA, CHIP_VAL_DPA, MERCH_WT_GT, and CHIP_WT_GT from tree_vol_val_by_species_diam_groups, and COMPLETE_CPA from harvest_costs. It looks like the merch wood revenue and energy wood revenue escalators are applied to MERCH_WT_GT and CHIP_WT_GT respectively.

jsfried commented 2 months ago

And does COMPLETE CPA have escalators in it? And the haul cost must also be a component of MAX_NR_DPA-- does that have escalators in it?

druepdx commented 2 months ago

The COMPLETE_CPA is directly from the harvest_costs table in processor results. Also, yes the merch and chip haul costs are components and that is actually where the escalators are applied before being multiplied by the MERCH/CHIP_WT_GT. The SQL statement is really long and a bit difficult to parse, but it looks like the revenue escalators are being applied to the haul costs, then those values are multiplied by the MERCH/CHIP_WT_GT.

lbross commented 2 months ago

I assume you mean that the cost escalators are applied to the haul costs? Revenue escalators would be applied to the tree_vol_val_by_species_diam_groups table in Processor.

jsfried commented 2 months ago

hopefully it is the COST escalators that are being applied to the haul costsnot the rev ones

druepdx commented 2 months ago

In the original SQL, the MerchWoodRevenue and EnergyWoodRevenue escalators are applied.

jsfried commented 2 months ago

Arrgh. Good catch

lbross commented 2 months ago

So the incorrect escalators are used in an additional place from where you fixed it last week? If so, let's fix this too then.

druepdx commented 2 months ago

I made those corrections and pushed the changes. A quick search didn't immediately return any other place where it looks like the incorrect escalators are used

lbross commented 1 month ago

OK! I'll switch our comments over to this issue instead of #354. I suspect the error lies in copying the diameter groups when a Processor scenario is copied. This was migrated to SQLite in v5.11.0 and I don't remember it being thoroughly vetted ... Thanks for the details though

druepdx commented 1 month ago

It does look like the diameter groups are being copied correctly. It's possible the error is happening when running the scenario

lbross commented 1 month ago

Good sleuthing. To answer your earlier question, the data in source scenario does NOT get copied over when a scenario is copied. We just create a empty scenario_results.accdb in the new, copied scenario directory. This might be a tough one to track down ...

druepdx commented 1 month ago

I noticed somewhere the escalators were being applied wrong in Optimizer. I'm not sure how since I had updated them all before, but I wonder if that was making a difference? I don't think Optimizer writes to that table at all though

lbross commented 1 month ago

That shouldn't cause the problems Jeremy is describing, but if that's the case, we need to fix this too. Perhaps something went wrong in the pull request or merge? If you can, please keep these fixes separate from the work you've been doing on uc_plot_input.cs. We don't want any of that going into the next v5.11.1 build.

druepdx commented 1 month ago

I only pushed my changes on uc_optimizer_scenario_run.cs so that should do the job regarding keeping everything separate

lbross commented 1 month ago

Sounds good.

jsfried commented 1 month ago

Were you able to duplicate my experience? Is there a workaround that I can use to get diameter classes to not be zero so that I can continue my validation of this issue?

druepdx commented 1 month ago

I wasn't able to duplicate the error. I'm not sure if @lbross has an idea for a workaround

lbross commented 1 month ago

I haven't had a chance to duplicate the error so no suggestions for a workaround yet. And we don't want Jeremy to continue validating the issue because Drue found another error with the escalators that is not incorporated into the current .msi. If Jeremy has time today, the most helpful thing would be to try to duplicate the error. If it doesn't happen again, then we can move on to building a new .msi with Drue's fix. Otherwise I will take a look when I have resolved the issues with Dr. Duh's project.

jsfried commented 1 month ago

Will do

lbross commented 1 month ago

I attempted to duplicate the error on the project cwi20240828 that Cat posted earlier this week. Here is what I did:

  1. Created new Processor scenario
  2. Copied existing scenario1 into new scenario
  3. Set the escalators using the default button and saved
  4. Ran Processor for all of the packages

My tree_vol_val_by_species_diam_groups had records for diam groups 1-5 and 999.

@druepdx: Please submit a pull request for your fixes to uc_optimizer_scenario_run.cs. I'll then try to publish a new .msi with this and the audit fix.

jsfried commented 1 month ago

I looked at the state of the tables in the processor rule definitions and in the scenario results and posted these in the temp30days issue352. While the table scenario_tree_diam_groups looked fine (an exact copy of scenario 1), the scenario_tree_species_diam_dollar_values table did not (all values for the Escalated scenario are 0): image

Moveover, all the diameter classes in the scenario_results.tree_vol_val_by_species_diam_groups show as 0: image

I will repeat the steps that generated this outcome and post the project in the same folder if we get the same result. Note that I reran processor for a subset of the packages because some of the packages are defunct (no longer wanted, but can't be zapped under the current BioSum version).

jsfried commented 1 month ago

It will take about an hour to run processor for the packages in this project, so it will be a little while before I can confirm whether I can duplicate the behavior. If it is not duplicated, then my rec is to exit processor after creating a new scenario, and again after copying the scenario (and saving the copy). And only then enter processor to run the scenario. I did check the scenario_tree_species_diam_dollar_values table and this time the values were copying over correctly (i.e., not all 0)

lbross commented 1 month ago

Having zeroes in the scenario_tree_diam_groups is definitely not going to give us the output we want in TreeVolVal. I just tried copying Processor scenarios for projects with and without an Access version of the scenario parameters, thinking that that might be it. But they were both fine. Crossing my fingers that your results are good. Hope to have a new .msi for you on Monday after I get Drue's code.

druepdx commented 1 month ago

I created a pull request for those Optimizer changes

jsfried commented 1 month ago

What change was made to the escalators in the new msi (to be built on Monday) so I can account for it when evaluating this release? Or is it better to wait for the new msi?

druepdx commented 1 month ago

They were corrected in Optimizer. For some reason, it looks like the corrections I had previously made weren't showing. The escalators should now be correctly applied to the econ_by_rx_cycle table in Optimizer results. The discrepancy wouldn't have affected anything in Processor though

jsfried commented 1 month ago

In that case, I will wait for the Monday msi since I already checked processor and am satisfied with how escalators are performing there (except for volval which I was not able to check but may be able to now when this processor run completes). Have a great weekend!

jsfried commented 1 month ago

Bad news-- I got 0s again in the vvbsdg table for diameter group for every record. Moreover, the values in the scenario_tree_species_diam_dollar_value table in processor rule definitions database are now 0 for all rows of this Escalated scenario (whereas they were NOT 0 before running Processor)! I will post the project

jsfried commented 1 month ago

I am posting two versions of this cwi project-- one with yesterday's date in the folder name (1003) and one with todays (1004). 1003 is the 5.11.0 version of the project. I started with that, loaded it into 5.11.1. Saved. Reopened. Went to processor. Made new scenario "Escalated"". Saved. Exited processor. Restarted processor. Opened Escalator scenario. Copied Scenario 1 into Escalator. Saved. Exited Processor. Restarted processor, loaded "Escalated" then ran packages 763, 764, 863, 873 and 874 as these are the only fully valid packages in this project. The result was the project with the 1004 in its folder name,

lbross commented 1 month ago

I was able to replicate this with the 1003 project. But only for one scenario. And it happened without changing the escalators, so that's not part of it. This is really strange. I'm going to keep digging.

This is a case-sensitivity issue with the scenario name: Escalated. BioSum stores the scenario name in lower case and 'Escalated' != 'escalated' unless you account for the letter case in the SQL statements. I'm not sure if it's always worked this way or if this was introduced in the SQLite conversion. In any case, I want to go through and check all of the Processor setting components to make sure they are case-insensitive. It might be a couple of days. In the meantime if you use all lower-case for scenario names, you should be fine.

jsfried commented 1 month ago

Lesley @lbross - are you saying that if I name the scenario all lower case that the diameter class 0 will no longer occur?

lbross commented 1 month ago

Yes. I don't know if renaming an existing scenario to all lower case will work. But creating a new scenario with a lower case name and copying from scenario1 should work. So far I've discovered, in addition to what you've already uncovered, that this affects the move-in costs and custom escalators. The default values will always be used for those two items.

jsfried commented 1 month ago

Great. I am a bit relucatant to invest time in validating the multipliers though until there is an msi that addresses potential glitches in how those worked in the optimizer module. Glad that you found the source of the trouble though (case sensitivity)

lbross commented 1 month ago

Please don't do any more validation on the current .msi that you have. I'm not sure how deep this goes and we want to preserve your time.

jsfried commented 1 month ago

no problem-- I am holding off and fighting other fires ;-)

druepdx commented 1 month ago

I don't think there should be a case problem with optimizer since some of the scenarios in the Douglas project have capitals, but I'll double check

druepdx commented 1 month ago

Optimizer seems to be fine regarding case sensitivity

lbross commented 1 month ago

Thanks @druepdx for checking on that. I'm afraid I'm going to need some more time with Processor before we're ready for a v5.11.1 release. In the meantime, can you work up a spreadsheet that you can use to test that the escalators are working as expected once we have a release? May be safest to work with the CWI project since that is most recent in @jsfried memory. What we need to do is pick a condition or two and manually calculate the fields (that use the escalators) that are in the econ_by_rxcycle table for each of the 4 cycles and make sure it matches what BioSum is putting out in the table. The current .msi will probably give you the wrong answer since we determined that the code is incorrect, but this is a test itself. Then you'll be ready to test the next .msi before we give it to Jeremy.