From e88d2a631995e4e174e742d45f7db0343b3291aa Mon Sep 17 00:00:00 2001 From: Michael Pivato Date: Fri, 23 Feb 2024 19:51:58 +1030 Subject: [PATCH] Fixes overhead allocation to consider lines that don't participate in overhead allocation, and fix from_cost_centre department --- src/overhead_allocation.rs | 136 ++++++++++++++++++++----------------- 1 file changed, 75 insertions(+), 61 deletions(-) diff --git a/src/overhead_allocation.rs b/src/overhead_allocation.rs index ac0dcd2..898a149 100644 --- a/src/overhead_allocation.rs +++ b/src/overhead_allocation.rs @@ -3,6 +3,7 @@ use std::{ io::Read, }; +use csv::Reader; use itertools::Itertools; use nalgebra::{DMatrix, Dynamic, LU}; use rayon::prelude::{IntoParallelRefIterator, ParallelIterator}; @@ -93,9 +94,6 @@ pub fn reciprocal_allocation, areas: &mut csv::Reader, cost_centres: &mut csv::Reader, - // TODO: Receiver method rather than this writer that can accept - // the raw float results, so we can write in an alternate format - // that more accurately represents the values on disk output: &mut impl RecordSerializer, use_numeric_accounts: bool, exclude_negative_allocation_statistics: bool, @@ -115,28 +113,8 @@ where .deserialize() .collect::, csv::Error>>()?; - let all_accounts_sorted: Vec = if use_numeric_accounts { - accounts - .deserialize::() - .filter(|account| { - account.is_ok() && account.as_ref().unwrap().account_type == account_type - }) - .map(|line| line.unwrap().code.clone().parse::().unwrap()) - .unique() - .sorted() - .map(|account| account.to_string()) - .collect() - } else { - accounts - .deserialize::() - .filter(|account| { - account.is_ok() && account.as_ref().unwrap().account_type == account_type - }) - .map(|line| line.unwrap().code.clone()) - .unique() - .sorted() - .collect() - }; + let all_accounts_sorted: Vec = + get_accounts_sorted(use_numeric_accounts, &account_type, accounts); let allocation_statistics = allocation_statistics .deserialize::() @@ -266,6 +244,7 @@ where let mut limited_ccs: Vec = Vec::new(); for limit_to in limit_tos.iter() { // TODO: It is technically possible to have more than one limit to (I think?) for a slot, so consider eventually splitting this and doing a foreach + // Also there's an exclude criteria that needs to be considered, which can exclude a rollup that would normally get included let limit_value = area.get(&(format!("LimitTo:{}", limit_to))).unwrap(); if limit_value.is_empty() { continue; @@ -293,35 +272,24 @@ where let mut totals: Vec<(String, String, f64)> = overhead_ccs .par_iter() .flat_map(|overhead_cc| { - let limited = limited_ccs + limited_ccs .iter() - .filter(|other_cc| { - totals.contains_key(&( - // TODO: This looks terrible - other_cc.clone().clone(), - allocation_statistic.clone(), - )) - }) - .map(|other_cc| { - ( - overhead_cc.clone(), - other_cc.clone(), - totals - .get(&(other_cc.clone(), allocation_statistic.clone())) - .map(|f| *f) - .unwrap(), - ) + .map(|other_cc| (other_cc.clone(), allocation_statistic.clone())) + .filter_map(|(other_cc, allocation_statistic)| { + let combined_stat = (other_cc, allocation_statistic); + if !totals.contains_key(&combined_stat) { + None + } else { + Some(( + overhead_cc.clone(), + combined_stat.0.clone(), + totals.get(&combined_stat).map(|f| *f).unwrap(), + )) + } }) .filter(|(_, _, value)| *value != 0.) .filter(|(from_cc, to_cc, _)| from_cc != to_cc) - .collect_vec(); - // TODO: Put me back if rayon proves problematic - // Insert is safe, since an overhead cc can only be a part of one area - // overhead_cc_totals.insert( - // overhead_cc.clone(), - // limited.iter().map(|(_, _, value)| value).sum(), - // ); - limited + .collect_vec() }) .collect(); overhead_other_total.append(&mut totals); @@ -355,24 +323,41 @@ where } // Export initial totals for operating departments - if show_from { - for line in lines.iter() { - if !overhead_ccs.contains(&line.department) { + for line in lines.iter() { + // TODO: Should we still output accounts that aren't in the accounts file anyway? + if all_accounts_sorted + .iter() + .find(|account| **account == line.account) + .is_some() + && !overhead_ccs.contains(&line.department) + && (show_from + // When we write out the final amounts rather than changes, + // ensure we still output departments that won't be receiving + // any costs. + || !overhead_other_total + .iter() + .any(|(_, to_department, _)| *to_department == line.department)) + { + if show_from { output.serialize(MovedAmount { account: line.account.clone(), cost_centre: line.department.clone(), value: line.value, from_cost_centre: line.department.clone(), })?; + } else { + output.serialize(CsvCost { + account: line.account.clone(), + department: line.department.clone(), + value: line.value, + })?; } } } // Finally, for each cc match total produced previously, sum the overhead cc where overhead cc appears in other cc, then // divide the other cc by this summed amount (thus getting the relative cost) - - // At this point we convert to our format that's actually used, need to somehow recover the to_cc_type... could build that out from the areas - + // At this point we convert to our format that's actually used in overhead allocation let allocation_rules: Vec = overhead_other_total .iter() .map( @@ -389,6 +374,8 @@ where ) .collect(); + // TODO: THIS CAN BE WRONG WHEN USING A FILE WITH ALL PASSES, for now ensure the input movement + // file only contains the final pass/outputs. let mut initial_account_costs: HashMap> = HashMap::new(); for line in lines { // Only include accounts we've already filtered on (i.e. by account type) @@ -430,7 +417,7 @@ where for cost in results { for department in cost.summed_department_costs { // Any consumers should assume missing cc/account value was 0 (we already ignore overhead, as they all 0 out) - if department.value > 0.00001 || department.value < -0.00001 { + if department.value != 0_f64 { output.serialize(CsvCost { account: cost.account.clone(), department: department.department, @@ -443,6 +430,35 @@ where Ok(()) } +fn get_accounts_sorted( + use_numeric_accounts: bool, + account_type: &String, + accounts: &mut Reader, +) -> Vec { + if use_numeric_accounts { + accounts + .deserialize::() + .filter(|account| { + account.is_ok() && account.as_ref().unwrap().account_type == *account_type + }) + .map(|line| line.unwrap().code.clone().parse::().unwrap()) + .unique() + .sorted() + .map(|account| account.to_string()) + .collect() + } else { + accounts + .deserialize::() + .filter(|account| { + account.is_ok() && account.as_ref().unwrap().account_type == *account_type + }) + .map(|line| line.unwrap().code.clone()) + .unique() + .sorted() + .collect() + } +} + fn split_allocation_statistic_range( allocation_statistic: &CsvAllocationStatistic, accounts_sorted: &Vec, @@ -661,7 +677,7 @@ fn solve_reciprocal_no_from( &operating_slice_costs, ); - // // Borrow so we don't move between loops + // Borrow so we don't move between loops let operating_overhead_mappings = &operating_overhead_mappings_mat; let calculated_overheads = &calculated_overheads; @@ -682,7 +698,6 @@ fn solve_reciprocal_no_from( // Redistribute floating point errors (only for ccs we actually allocated from/to) // Considered removing this since redistribution should be done in cost driver calculations, however since that usually // does nothing, we may as well keep this just in case. - // TODO: Not sure we actually need this, would probably be better to have a better storage format than // csv/string conversions // let initial_cost: f64 = total_costs @@ -696,7 +711,6 @@ fn solve_reciprocal_no_from( // .sum(); // let new_cost: f64 = converted_result.iter().map(|cost| cost.value).sum(); // let diff = initial_cost - new_cost; - AccountCost { account: total_costs.account.clone(), summed_department_costs: converted_result @@ -753,7 +767,7 @@ fn solve_reciprocal_with_from( account: total_costs.account.clone(), cost_centre: department.clone(), value, - from_cost_centre: department.clone(), + from_cost_centre: overhead_department_cost.department.clone(), }) .filter(|cost| cost.value != 0_f64) .collect::>()