Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Analytical_Engine + Graphics_Engine: Project graph to dependency diagram. #2198

Merged
merged 16 commits into from
Dec 7, 2020
Merged

Analytical_Engine + Graphics_Engine: Project graph to dependency diagram. #2198

merged 16 commits into from
Dec 7, 2020

Conversation

rolyhudson
Copy link
Contributor

@rolyhudson rolyhudson commented Nov 30, 2020 •
edited
Loading

@rolyhudson rolyhudson added the type:feature New capability or enhancement label Nov 30, 2020
@rolyhudson rolyhudson self-assigned this Nov 30, 2020
@rolyhudson rolyhudson added the status:WIP PR in progress and still in draft, not ready for formal review label Nov 30, 2020
@rolyhudson rolyhudson changed the title Analytical o m#1079 non spatial graph 2 Analytical_Engine Graphics_Engine methods to project graph to dependency diagram Nov 30, 2020
@rolyhudson rolyhudson changed the title Analytical_Engine Graphics_Engine methods to project graph to dependency diagram Analytical_Engine + Graphics_Engine: methods to project graph to dependency diagram. Nov 30, 2020
@rolyhudson rolyhudson changed the title Analytical_Engine + Graphics_Engine: methods to project graph to dependency diagram. Analytical_Engine + Graphics_Engine: Project graph to dependency diagram. Nov 30, 2020
@rolyhudson rolyhudson requested a review from alelom November 30, 2020 15:03
@rolyhudson
Copy link
Contributor Author

@al-fisher + @alelom just to note that the stuff we looked at this morning is here. @al-fisher the file IElementDependencies.gh is now in the SS folder of the test files folder in case it is needed for reference.

@al-fisher
Copy link
Member

Thanks @rolyhudson

al-fisher
al-fisher reviewed Dec 4, 2020
View reviewed changes
Copy link
Member

@al-fisher al-fisher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is looking really good. Can see this scaling well.

I do have a couple of thoughts around conventions and the precise flow of information.
Namely the role/split of responsibility between the View, the Projection and the act of Projecting method.

image

What is the requirement of the intermediate layer of the GraphicalProjection - could Views or similar not be fed directly into the IProjectGraph method itself. Is this to help manage dependency between Analytical and Graphical ?
There are multiple layers of terminology here which will be nice to clarify and even simplify if we can.

Functionality wise - I do really like the way it is working - along with the various RepresentationFragments.

@al-fisher al-fisher mentioned this pull request Dec 4, 2020
Open
al-fisher
al-fisher approved these changes Dec 4, 2020
View reviewed changes
Copy link
Member

@al-fisher al-fisher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As discussed @rolyhudson - raised issue here BHoM/BHoM#1120

Very happy this PR creates the functionality we need to continue to prototype out and start using. Any potential name changes can be actioned at later date.

Happy to approve to merge and continue experimentation

@al-fisher
Copy link
Member

/azp run BHoM_Engine.CheckInstaller

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

al-fisher
al-fisher approved these changes Dec 7, 2020
View reviewed changes
Copy link
Member

@al-fisher al-fisher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reapproving inline with BHoM/BHoM#1109
LGTM

@al-fisher
Copy link
Member

/azp run BHoM_Engine.CheckInstaller

@al-fisher al-fisher removed the status:WIP PR in progress and still in draft, not ready for formal review label Dec 7, 2020
@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@al-fisher al-fisher merged commit 173885e into master Dec 7, 2020
@al-fisher al-fisher deleted the Analytical_oM#1079_NonSpatialGraph_2 branch December 7, 2020 09:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@al-fisher al-fisher al-fisher approved these changes

@adecler adecler Awaiting requested review from adecler adecler is a code owner

@alelom alelom Awaiting requested review from alelom

Assignees

@rolyhudson rolyhudson

Labels
type:feature New capability or enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Analytic_Engine: add methods to support work flows for non-spatial views of Graphs
2 participants
@rolyhudson @al-fisher