USFS-PNW / Fia-Biosum-Manager

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

PROCESSOR: BioSum crashes if an additional harvest cost is defined at the rx-level but not in the FVS_COMPUTE table #354

Closed lbross closed 1 week ago

lbross commented 2 months ago

This should be a valid configuration for legacy rx-level harvest cost processing. Fix the bug in DoesVariantPackageUseKcpCpa() and ensure that rx-level harvest costs work as they did before.

jsfried commented 2 months ago

It is not so much a legacy thing a a simpler way of specifying per acre costs that will not vary by stand-- for example, if mastication is part of the treatment specfication it will happen every time trees are cut. Road maintenance and waterbarring associated with opening up operations on a stand might also be imposed whenever a stand is treated regardless of what happens with the amount of tree cutting and dead wood generating and burning on the acre. Less sophisticated BioSum/FVS users will appreciate being able to account for those costs, or even to account for burning costs more crudely (not sensitive to how much fuel is there to be removed) as it requires less sophistication in the FVS part of the simulation

lbross commented 2 months ago

This is fixed. When I fixed the glitch that required the additional harvest cost to be in the FVS_COMPUTE table, the rx-level additional harvest cost processing worked as has in the past.

To validate you will need to set up rx level costs in the Processor additional harvest cost screen and NOT use the fvs_compute table to control their application.

Cat-Ducat-USDA commented 1 month ago

To validate, I set up an rx-level additional cost component (called pLANT) for two of the four packages in the run. When I go to set the $350 value in the processor workflow, I keep getting an error message, no matter what I select from the dropdown menu (i.e., Edit all values, assign to all components, etc.), so the cost is not getting assigned. I have attached the error message below. ![Uploading BioSum_Issue354Val.PNG…]()

lbross commented 1 month ago

I will need the project for this one. I just followed the same steps (I think) and was able to assign the costs. Also, the image upload didn't come through. Maybe it wasn't done uploading before you saved the issue? I'm going to hold off on the new .msi until we have a chance to get this sorted.

Cat-Ducat-USDA commented 1 month ago

Oop, meant to add that, sorry. I will do that now. Yeah, I dont know why the image wouldn't work but the error message says "Module - SQLite:SqlNonQuery Err Msg - SQL logic error no such table: additional_harvest_costs_work_table the SQL command UPDATE additional_harvest_costs_work_table SET pLANT=350.00 WHERE rx='763' Failed

Cat-Ducat-USDA commented 1 month ago

Project, called cwi20240824 (I promise we are going to improve the naming...), is uploading in Temp30Days. folder

lbross commented 1 month ago

The project inside the zip file was called cwi20240709. It didn't have a harvest cost 'pLANT' defined for rx 763. I added one and it worked fine. Did you zip up the right project? The error message kind of makes sense, but I so far I haven't been able to recreate it so I can't fix it.

Cat-Ducat-USDA commented 1 month ago

Hey Lesley, yeah it's definitely the correct project. The folder inside should say cwi20240709... that's the main project name. The outer folder is one of the versions of the project (time stamped with the date it was created). cwi20240824 is an older version of the CWI project. I am not sure why pLANT didn't transfer over... I still see it on my end. I am going to try and rerun through processor now.

Cat-Ducat-USDA commented 1 month ago

Well, I figured out my issue. I was using the wrong option in the dropdown menu for assign costs... have to use the assign to NULL values, not to all occurences. So, costs have now been assigned and I am running processor to finish validating this! Thanks Lesley, sorry for the confusion! I'll let you know shortly how the processor outputs look.

jsfried commented 1 month ago

I had not thought of that, but it does make sense. Because BioSum requires that the cost be defined (column named/created) back at the Rx level, then it stands to reason that BioSum only wants to assign costs for the cost columns that have been defined-- trying to assign for all prescription-stand-cycle combinations when the cost column is only defined for a subset of them is clearly going to induce indigestion (or an error message)! Perhaps Cat will identify some good places in the documentation to convey this requirement-- perhaps both in the Rx section and in the Processor section where cost assignments are described... @Cat-Ducat-USDA

Cat-Ducat-USDA commented 1 month ago

Yes, I have already made notes to add on that language! So, after running processor, still not seeing the 350 come up anywhere in the additional_cpa column, nor am I seeing anywhere where the difference between the harvestcpa and completecpa where the difference is 350. Seems like it did not work.

lbross commented 1 month ago

Please repost the project in its current state and I'll take a look tomorrow.

Cat-Ducat-USDA commented 1 month ago

We figured it out, I believe. Switched back to assigning cost component to all component occurrences, which for whatever reason, worked this time around. We did an experiment where we eliminated the flagged components (pILE and bROAD) from rx 763, and the run was successful, with the pLANT values populating the harvest_costs table correctly.

It appears that one cannot have both a flagged and an unflagged cost in the same Rx. We did find that the columns for pILE and bROAD were retained in the scenario_rule_definitions table, but I imagine that's because there were still other Rx's that had those flags.

lbross commented 1 month ago

That is correct. Flag-driven and Rx driven costs cannot exist in the same Rx. It's been hard enough to get them both working independently, let alone coordinating together :-) Glad you were able to get it working. It is complicated how all of the pieces fit together. Let me know your thoughts about the next .msi to fix the pre-audit error. Do you want to do some more validations to make "sure" there's nothing else to include?

jsfried commented 1 month ago

I have, I think, one more validation to complete, but it is a douzy (the correction to the escalator implementation). Am literally tackling that one now. I think Cat has completed all of hers, so as soon as I complete this one, I think we're validated.

lbross commented 1 month ago

I had forgotten about that one. Yes, let's wait. Because if that fix isn't complete, any updates definitely need to be included in a v5.11.1 release. Thanks!

jsfried commented 1 month ago

Do you or Drue recall whether the fixes change anything in processor (and its scenario_results) or only in optimizer (and its scenario results)? That will be helpful for my testing

lbross commented 1 month ago

I just checked the code log and there are no changes in Processor related to the incorrect escalators. However, issue #349, the inclusion of plots with invalid yarding distances in the Optimizer output, does require that you rerun Processor as we're now catching these plots before they get to Optimizer.

jsfried commented 1 month ago

So what I have learned so far about processor output by comparing a 5.11.0 run with escalators set to 1 and a 5.11.1 run with escalators set to 0.5, 0.4 and 0.3 for costs, merch rev and chip rev, respectively, for cycles 3 and 4 only:

  1. Harvest_cpa is unaffected by escalators
  2. Additional_cpa IS affected (scaled) by escalators
  3. Chip_cpa is not affected by escalators
  4. complete_cpa IS affected by escalators (both the harvest_Cpa and the additional_cpa get scaled and then summed to yield this complete_cpa
  5. additional_kcp_cpa values do get scaled by escalators

I went to check things in the VolValBySppDiam table and ran into a problem. All diameter groups in this new scenario that I had built (by copying the existing scenario and then adding escalators) were either 0 or 999-- not sure why that would be, but until that is resolved I cannot make this comparison.

lbross commented 1 month ago

@druepdx Can you take a look at the CopyScenario function in Processor? It should be similar to the one in Optimizer. I have a hot issue for Dr. Duh and am not sure how soon I can get to this. Let me know if you have questions

druepdx commented 1 month ago

Yes, I can try to work on it

jsfried commented 1 month ago

For what it is worth, I was just using the latest CWI project, but read into and upgraded by 5.11.1 and using Scenario 1 to copy into a new processor scenario into which I reset the escalators...

jsfried commented 1 month ago

Sorry this chain should be in 352 not 354-- my bad

druepdx commented 1 month ago

Is the issue actually occurring when the scenario is copied or when you run the copied scenario with different escalators? I don't think the data in the results tables gets copied over

jsfried commented 1 month ago

I copied the scenario. Then I entered new escalators (not 1s). Then I saved. Then I ran. Then I ran optimizer.

druepdx commented 1 month ago

If I understand correctly, I think the issue is happening when running the processor scenario, not when copying it. I think CopyScenario in Processor is working correctly