Improve performance of EvaluateMethods by Eliminating redundant method calls

enarhi commented 3 years ago

Description:

Performance currently takes a hit due to multiple calls to heavier methods such as MaterialComposition. A very light refactoring of the evaluate methods can eliminate this redundancy, ensuring we only make one MaterialComposition call (along with any other duplicates) to find the relevant materials, quantities and values and calculate accordingly.

michaelhoehn commented 2 years ago

I had a look through the compute methods as a part of making sure everything is in line for https://github.com/BHoM/LifeCycleAssessment_Toolkit/pull/265. And after looking through closely, I don't really see any unnecessary redundancies. I've taken this snippet from EvaluateEPDByMass() because I can see how this would look pretty redundant.

So here MaterialComposition() is called strictly to get a count of materials on the elements so that the ratios can be accurately matched back to the potentially, very large collection of objects without any issues. Thus for(int i = 0; i < matList.Count(); i++) gives us all of the right materials with "volume" quantity type && a corresponding density value based on the material's overall ratio profile. Removal of these calls would not match materials to metrics accurately.

It would be helpful if you could point out specific instances where you find redundancies that affect performance that I may have missed!

FYI @FraserGreenroyd would be good to have your eyes on this issue as well. Either helping spot redundant calls and/or resolution on this together.


            List<double> gwpByMaterial = new List<double>();
            MaterialComposition mc = elementM.IMaterialComposition();
            List<Material> matList = mc.Materials.ToList();

            List<double> epdVals = elementM.GetEvaluationValue(field, phases, QuantityType.Mass, exactMatch);
            if (epdVals == null || epdVals.Where(x => !double.IsNaN(x)).Sum() <= 0)
            {
                BH.Engine.Base.Compute.RecordError($"No value for {field} can be found within the supplied EPD.");
                return null;
            }

            for (int i = 0; i < matList.Count(); i++)
            {
                double density;
                List<EnvironmentalProductDeclaration> materialEPDs = matList[i].Properties.OfType<EnvironmentalProductDeclaration>().ToList();
                if (materialEPDs.Any(x => x.QuantityType == QuantityType.Mass))
                {
                    double volumeOfMaterial = mc.Ratios[i] * volume;
                    List<double> densityOfMassEpd = materialEPDs.Where(x => x.QuantityType == QuantityType.Mass).First().GetEPDDensity();
                    if (densityOfMassEpd == null || densityOfMassEpd.Count() == 0)
                    {
                        BH.Engine.Base.Compute.RecordError("Density could not be found. Add DensityFragment for all objects using Mass-based QuantityType EPDs.");
                        return null;
                    }
                    else
                        density = densityOfMassEpd[0];

                    double massOfObj = volumeOfMaterial * density;
                    if (double.IsNaN(epdVals[i]))
                        gwpByMaterial.Add(double.NaN);
                    else
                        gwpByMaterial.Add(epdVals[i] * massOfObj);
                }
            }

enarhi commented 2 years ago

Along the chain of methods that EvaluateEnvironmentProductDeclaration calls, 'IMaterialCompositionis called numerous times. Examples of this are withinGetQuantityType`:

And twice within GetElementEPD:

Also within GetEvaluationValue:

I recommend we only call IMaterialComposition at the parent level in the EvaluateEnvironmentalProductDeclaration method. By calling it here, we can then select materials per their epd type and only send those along with their metric (volume, mass, etc) to the respective Evaluate methods (such as EvaluateEnvironmentalProductDeclarationByMass. These methods will then have new overloads that work on a per material+volume/mass/length/area basis (meaning the current methods parameter for elementM for that method would be replaced with those two parameters instead). This would lead to one material composition call, with all other methods in the chain replacing that method with more specific calls.

A concrete example of this would be that an element with two layers, one with a mass-based epd and one with a volume-base epd, would work like so:
EvaluateEnvironmentalProduct gets the MaterialComposition of the Element.
Each Material from MaterialComposition is fed through the current QuantityType switch, switching based on Material.QuantityType' (This can be a new method for QuantityType that will getQuantityType` per material, super fast)
Corresponding Evaluate method will be called by the switch, and within that switch the specific Mass/Volume/etc calc will be done. For example, for Mass, within: before the method is called we will do double mass = Ratio*Volume*EPDDensity (EPDDensity here will be quick to get as it will be per the specific Material, not the element, thereby eleminating that IMaterialComposition call). -From here the method with the new overload will get the Evaluation value as before, but without requiring the IMaterialComposition call. Everything after that remains as is.