-
Notifications
You must be signed in to change notification settings - Fork 15
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Matter_Engine: Add material mapping methods #2932
Matter_Engine: Add material mapping methods #2932
Conversation
@BHoMBot check compliance |
@IsakNaslundBh to confirm, the following actions are now queued:
|
@IsakNaslundBh to confirm, the following actions are now queued:
|
@IsakNaslundBh to confirm, the following actions are now queued:
|
@michaelhoehn this should be good to go for testing now. Happy to have a quick call if needed. Will add UTs to this PR once the approach taken in the PR has been approved and agreed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Finally got a chance to review this one- love the template, really simple to use. One issue on my end, when assigning a material from a traditional EPD dataset vs the concatenated one you provided, I received the following errors:
Is this because those datasets are missing the new material attribute?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm happy with the additions, but I think Kayleigh's observations should be addressed, likely in the LCA toolkit PR. I'm fine to see this merged personally, but I do find it interesting and somewhat concerning that a user may instinctually combine or "inject" another elementM upstream of the solver (which is what I'm assuming was done here, @kayleighhoude please correct me if I'm mistaken) which do not have mapped data, to begin with.
In this case the methods will produce a null value for anything that is not properly mapped (as they should) and produce typical results for those that have mapped data. I'll add some additional thoughts on the LCA PR referenced above to cover these cases. But perhaps there's some null handling to be added here?
Please see the comment here BHoM/LifeCycleAssessment_Toolkit#288 (review) |
…her Material matching based on name and/or name of properties
…s check and mapping should be done
…the mapping clearer
… templates for compositions and takeoffs Previous version worked with lists to aim to achieve some speeder run of the methods, but made them harder to work with. Changing to run element by element, as this is a relatively quick process anyway
0ce6854
to
f51c4fe
Compare
Branch naming inconsistencies have required me to fork this branch for compliance purposes FYI @FraserGreenroyd |
@BHoMBot check core |
@michaelhoehn to confirm, the following actions are now queued:
|
@BHoMBot check required |
@michaelhoehn to confirm, the following actions are now queued:
|
The check |
The check |
@BHoMBot check documentation-compliance |
@michaelhoehn to confirm, the following actions are now queued:
|
@BHoMBot check required |
@michaelhoehn to confirm, the following actions are now queued:
|
The check |
@FraserGreenroyd to confirm, the following actions are now queued:
|
@BHoMBot check ready-to-merge |
@FraserGreenroyd to confirm, the following actions are now queued:
There are 8 requests in the queue ahead of you. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See here.
Issues addressed by this PR
Closes #2900
Test files
https://burohappold.sharepoint.com/:f:/s/BHoM/ElafUY8_5_dOs1UN37qQ1kQBFOo2qUahCMBTEbp2g7uEag?e=rRgCke
Changelog
Additional comments