LCA_Toolkit: Updated EvaluateEPD method to handle mixed EPD quantity types

[vgreen-BH]

Issues addressed by this PR

Closes #239

Fixes how EvaluateEnvironmentalProductDeclarationByMass handles cases with multiple EPDs of mixed quantity types. This allows EvaluteEnvironmentalProductDeclaration to be run for objects with multiple EPD types.

Test files

https://burohappold.sharepoint.com/:f:/r/sites/BHoM/02_Current/12_Scripts/02_Pull%20Request/BHoM/LifeCycleAssessment_Toolkit/%23239-EvaluateQuantitybyEPD?csf=1&web=1&e=1OoVog

[michaelhoehn]

Looks good on initial review. Minor change requested in the record error message. Will test tomorrow and make sure it's all working 🚀

[enarhi]

LGTM

[michaelhoehn]

In addition to the comment inline below, I'm also going to request a new sample script for testing purposes as I don't think it's functioning as planned and the custom clusters obfuscate the procedure you're using to get to the result which makes thorough testing difficult.

You will see in my snapshot below that the first cluster is not returning the expected EPD type, therefore downstream from this your results aren't actually incorporating the changes you've made to the code (i.e. eval objects with multiple QTs). Please have another pass at making a simplified sample script. You can also make use of the extension methods to identify these data points at different stages of the script.

[enarhi]

@michaelhoehn I just tweaked the test script at the saved location to match with what I used to test on my end, give that a try.

[michaelhoehn]

Sorry if confusing or not clear, I put this in the wrong place in the diff but referred to it in the comment.
List<double> densityOfMassEpd = materialEPDs.Where(x => x.QuantityType == QuantityType.Mass).First().GetEPDDensity()

Works the exact same as what's propsed so happy to approve as it. Just wanted to make sure we weren't dropping anything so I made a case with lots of different QTs with good results. Nice one @vgreen-BH!

[FraserGreenroyd]

Looking at the Quantity Type enum, one option could be Volume.

If there are 3 materialEPDs, with item 0 being a volume, item 1 being a length, and item 2 being mass then the if statement will pass (which is asking if any of the EPD Quantity Types are of type mass), but this line will then take the first item which has a quantity type of volume.

Therefore, the variable densityOfMassEpd is inaccurate, because it would be a densityOfVolumeEpd instead.

This is why I think @michaelhoehn suggested code makes more sense - filter down to just the mass quantity types, take the first result and then get the density EPD from it. Owing to the way the code is written and how it is read (baring in mind I am not an LCA expert, so I'm just going by what I can see in the code), this will result in more confusion by anyone reading the code later.

Therefore we need to do 2 things in my view:

• Update the code to use @michaelhoehn suggestion to filter down and take the mass type
• Add code comments that explain what we as developers currently are trying to achieve - that will make it easier in 12 months if we come back to this code to debug to understand what we were thinking at the time we wrote it originally, without risking breaking stuff too much in the future

[michaelhoehn]

@BHoMBot check compliance

[bhombot-ci]

@michaelhoehn to confirm, the following checks are now queued:

• code-compliance
• documentation-compliance
• project-compliance
• branch-compliance
• dataset-compliance
• copyright-compliance

[michaelhoehn]

@BHoMBot check installer

[bhombot-ci]

@michaelhoehn to confirm, the following checks are now queued:

• installer

[michaelhoehn]

@BHoMBot check core

[bhombot-ci]

@michaelhoehn to confirm, the following checks are now queued:

• core

There are 2 requests in the queue ahead of you.

[michaelhoehn]

@BHoMBot check versioning

[bhombot-ci]

@michaelhoehn to confirm, the following checks are now queued:

• versioning

There are 4 requests in the queue ahead of you.

[bhombot-ci]

Please be advised that the check with reference 3615427063 has more than 50 annotations of notes. API limitations restrict annotations to 50. You may need to rerun this check to obtain the next set when you make changes. At the time of reporting this check, there are 792 additional annotations waiting, made up of 792 errors and 0 warnings.

[FraserGreenroyd]

Looking at the Quantity Type enum, one option could be Volume.

If there are 3 materialEPDs, with item 0 being a volume, item 1 being a length, and item 2 being mass then the if statement will pass (which is asking if any of the EPD Quantity Types are of type mass), but this line will then take the first item which has a quantity type of volume.

Therefore, the variable densityOfMassEpd is inaccurate, because it would be a densityOfVolumeEpd instead.

This is why I think @michaelhoehn suggested code makes more sense - filter down to just the mass quantity types, take the first result and then get the density EPD from it. Owing to the way the code is written and how it is read (baring in mind I am not an LCA expert, so I'm just going by what I can see in the code), this will result in more confusion by anyone reading the code later.

Therefore we need to do 2 things in my view:

• Update the code to use @michaelhoehn suggestion to filter down and take the mass type
• Add code comments that explain what we as developers currently are trying to achieve - that will make it easier in 12 months if we come back to this code to debug to understand what we were thinking at the time we wrote it originally, without risking breaking stuff too much in the future

[michaelhoehn]

@BHoMBot check core

[bhombot-ci]

@michaelhoehn to confirm, the following checks are now queued:

• core

[michaelhoehn]

@vgreen-BH please address the requested changes from @FraserGreenroyd as soon as you can as this is approaching the time limit for getting in the next beta as the code will need to be tested in an alpha prior to full public release.

Again I see that the implemented changes do fix the bug, and therefore will stand by my approval. However, I still recommend the suggested change to make absolutely sure you're always filtering down rather than relying on the first list entry in case anything slips past that is unexpected.

Regardless I've run all the required CI checks for this to be prepared to merge with success! As soon as there's some address of the above commentation we will go for merge and begin testing.

[enarhi]

@michaelhoehn @FraserGreenroyd comments picked up in the latest commit with a slight tweak to make the code mor ereadable and clearly set out the mass filtering followed by gathering of key calc values. I've updated the test file as well with an added section to test error handling when the metric is 0, density is not provided, no epd's are mass quantity type, etc.

[michaelhoehn]

LGTM

[michaelhoehn]

@FraserGreenroyd introduced additional workflow-breaking options into the logic built into the sample script. I've updated it following our conversation if you'd like to take a look. I'll also briefly summarise here:

• GetQuantityType() only returns the first QT when a single layer is comprised of several material properties as pictured below. This is different from at least my previous understanding of a "standard" material property construction which uses a singular material property rather than a list of properties, which does work as expected.
• Non-epd materials will throw an error regardless of the presence of epds with valid inputs using either method. This is endemic of a wider LCA-toolkit problem that @FraserGreenroyd is also referring to.

This example shows a few more of the recognised errors

• More sufficient null handling should remain in the solution, and be added to the public eval method to catch non-epds
• GetQuantityType() should handle a list of QTs natively, but tweaks need to be made to remove firstOrDefault() on L52

qt = elementM.IMaterialComposition().Materials.Where(x => x != null).Select(x => { var epd = x.Properties.Where(y => y is EnvironmentalProductDeclaration).FirstOrDefault() as EnvironmentalProductDeclaration; if (epd != null) return epd.QuantityType; return QuantityType.Undefined; }).ToList();

Agreed with @FraserGreenroyd that it's likely time we had a thorough workshop to bring a bit more cohesiveness to the toolkit as a whole prior to tossing in a hotfix the day before beta production on a bug that has been in the toolkit since last beta. Unfortunately, the bug will remain for another quarter publically, but I'm sure we can arrive at a better solution together soon that incorporates needed changes throughout the toolkit.

I should also add that I didn't try rolling back the last commit to verify whether or not the list errors were more recently introduced.

[JosefTaylor]

I pulled this branch, reverted Erik's commit, implemented Michael's suggestion, (and also switched to .Net 4.7.2) and it worked great. I can push my branch, but it's probably better someone who knows what they're doing does it.

[FraserGreenroyd]

I pulled this branch, reverted Erik's commit, implemented Michael's suggestion, (and also switched to .Net 4.7.2) and it worked great. I can push my branch, but it's probably better someone who knows what they're doing does it.

I would recommend not pushing that, the toolkit was upgraded to .Net 4.7.2 in this PR, it would be better to rebase onto main for whomever ends up picking this up.

[michaelhoehn]

Thanks @JosefTaylor for testing. Now that I'm back to work I'll pick up this PR as it seems to have gone a bit stale. I'll need perform the rebase and get it in line with the comments above. I will likely ask @JosefTaylor to stand in as reviewer seeing as most of the regulars have committed to this branch already 😄

[michaelhoehn]

The branch has been reverted and rebased successfully. I'll be editing the code based on the above discussion and look to merge this for further application. FYI @FraserGreenroyd

[michaelhoehn]

@FraserGreenroyd I've updated the code per suggestions above and have documented the additional findings in a new issue here #258

I move to merge this in as it does fix the proposed bug. Thankfully our discussion here did identify another pre-existing bug in that multiple Properties-based Materials would not function correctly.

I'd appreciate a quick review of the code and test script @FraserGreenroyd and @JosefTaylor at this point.

[JosefTaylor]

It works!

[michaelhoehn]

@BHoMBot check core
@BHoMBot check compliance
@BHoMBot check installer

[bhombot-ci]

@michaelhoehn to confirm, the following checks are now queued:

• core
• code-compliance
• documentation-compliance
• project-compliance
• branch-compliance
• dataset-compliance
• copyright-compliance
• installer

There are 43 requests in the queue ahead of you.

[michaelhoehn]

@BHoMBot check null-handling
@BHoMBot check serialisation
@BHoMBot check versioning

[bhombot-ci]

@michaelhoehn to confirm, the following checks are now queued:

• serialisation

[bhombot-ci]

@michaelhoehn to confirm, the following checks are now queued:

• null-handling
• serialisation
• versioning

There are 2 requests in the queue ahead of you.

[FraserGreenroyd]

LGTM, good work @michaelhoehn thanks for addressing the comments I had 😄

[michaelhoehn]

@BHoMBot check ready-to-merge

[bhombot-ci]

@michaelhoehn to confirm, the following checks are now queued:

• ready-to-merge

There are 37 requests in the queue ahead of you.