-
Notifications
You must be signed in to change notification settings - Fork 2
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
Move engine methods utilising serialisation to Adapter Execute #153
Move engine methods utilising serialisation to Adapter Execute #153
Conversation
…ustom one in to_bhom.py
… to use Point3D to_dict() method rather than to_bhom.
@Tom-Kingstone to confirm, the following actions are now queued:
|
@BHoMBot check core |
@Tom-Kingstone to confirm, the following actions are now queued:
|
@BHoMBot check compliance |
@Tom-Kingstone to confirm, the following actions are now queued:
|
@Tom-Kingstone to confirm, the following actions are now queued:
|
The check |
The check |
The check |
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.
As discussed on a call, Engine shouldn't use Adapter - @Tom-Kingstone will take a look at using Execute options instead and update the PR accordingly tomorrow/later int he week 😄
@Tom-Kingstone to confirm, the following actions are now queued:
|
@Tom-Kingstone fix requested for copyright headers. The errors with the copyright headers on the CS ( I will apply the fixes to every case detailed on the checks tab. If you want to perform the fixes in a different manner please resolve this manually and rerun the check. Each CS ( If you are happy for me to go ahead and perform this action, please reply with:
|
@BHoMBot fix copyright headers ref. 19180266457 |
@Tom-Kingstone I have queued up your request to fix copyright headers. There are 0 requests in the queue ahead of you. |
@Tom-Kingstone I am now going to fix the copyright compliance in accordance with the annotations previously made. |
@Tom-Kingstone to confirm I have now resolved the copyright compliance and pushed a commit to this Pull Request. |
@Tom-Kingstone to confirm, the following actions are now queued:
|
@BHoMBot check versioning |
@Tom-Kingstone to confirm, the following actions are now queued:
|
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 think the adapter references, of which only Adapter_oM
is needed, should have gone into the oM project, not the engine project?
@Tom-Kingstone to confirm, the following actions are now queued:
|
@Tom-Kingstone to confirm, the following actions are now queued:
|
The check |
The check |
The check |
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.
It's not great with the global variable but as discussed offline, not a lot we can do right now, so we'll merge this as a working solution for alpha testing and we can have more chats on improvements @Tom-Kingstone
@Tom-Kingstone to confirm, the following actions are now queued:
|
@BHoMBot check ready-to-merge |
@Tom-Kingstone to confirm, the following actions are now queued:
|
NOTE: Depends on
Issues addressed by this PR
Closes #143
The engine methods:
GetMaterial
,GetTypology
,SimulationResult
, andExternalComfort
have been converted to adapter methods callable byExecute
commands, that now utilise the LadybugToolsAdapter serialisation methods.Test files
LBT_Tests.zip
Ensure the EPW file path is valid, then set toggles to True in order (
GetMaterial
,GetTypology
,SimulationResult
,ExternalComfort
) and wait for them to finish computing, check for any warnings or errors, and finally check that the returning objects contain data in each property - This is testing whether the serialiser is pulling and pushing correctly.Additional comments
You must have pollination version 1.35.14 installed for these components to work - the methods check for this.