Skip to content

chore(architecture diagrams): add C4 diagrams#8

Open
Bai-Li-NOAA wants to merge 2 commits intomainfrom
add-architecture-diagrams
Open

chore(architecture diagrams): add C4 diagrams#8
Bai-Li-NOAA wants to merge 2 commits intomainfrom
add-architecture-diagrams

Conversation

@Bai-Li-NOAA
Copy link
Collaborator

What is the feature?

  • add a C4 system context diagram to present the ecosystem simulation toolkit: {ecosystemom} and {ecosystemdata}

How have you implemented the solution?

  • Add a mermaid flowchart to architecture_diagrams/system_context_diagram.md
  • Add /architecture_diagrams folder to .Rbuildignore
  • TODO: ideally, an arrow should connect DSEM and the stock assessment models. However, it's unclear how to force a clean left-to-right layout in Mermaid. The approach I tried results in a cluttered diagram.

Does the PR impact any other area of the project, maybe another repo?

  • Once merged into main, @Bai-Li-NOAA will update the {ecosystemdata} README to include a link to the diagram.

* add a C4 system context diagram
@kellijohnson-NOAA
Copy link
Contributor

@Bai-Li-NOAA gemini helped me figure out how to get rid of the titles in the subgraph, it is expecting a string not ""\n so if you replace the string with "" but add a space before the return then it will render without a subgraph title.

Copy link
Contributor

@kellijohnson-NOAA kellijohnson-NOAA left a comment

Choose a reason for hiding this comment

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

I tried to get the link between DSEM and SA but like your trials and tribulations I was unable to make it happen without changing the entire structure of the diagram. But, I do not think that we need all three external systems in the separate box, especially EWE models with DSEM and SA. I think that the legend helps define what they are and they do not need to be grouped together.
Also, I am unsure why some lines are dashed and some are solid.

@@ -0,0 +1,70 @@
# System context diagram

This diagram presents the ecosystem simulation toolkit at the center, surrounded by its users and the external systems
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
This diagram presents the ecosystem simulation toolkit at the center, surrounded by its users and the external systems
This diagram presents the ecosystem-simulation toolkit at the center, surrounded by its users and the external systems

Comment on lines +28 to +30
U1[<b>User 1</b><br/><br/>-<i>Person</i>-<br/><br/>Ecosystem Modeler]:::lightblue
U2[<b>User 2</b><br/><br/>-<i>Person</i>-<br/><br/>General Data User]:::lightblue
U3[<b>User 3</b><br/><br/>-<i>Person</i>-<br/><br/>Stock Assessment Modeler]:::lightblue
Copy link
Contributor

Choose a reason for hiding this comment

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

I would prefer to take out the -<i>Person<\i> from each of these lines because it is clear from the legend that it is a "person" and it would allow for more room for the diagram. Same for -<i>External System</i>- on lines 43--35.

%% Define external systems
subgraph ""
direction TB
EM[<b>Ecosystem Models</b><br/><br/> -<i>External System</i>-<br/><br/> Simulate ecosystem dynamics and produce raw outputs]:::lightpurple
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we list EWE and Atlantis here instead of "External System"?

subgraph ""
direction TB
EM[<b>Ecosystem Models</b><br/><br/> -<i>External System</i>-<br/><br/> Simulate ecosystem dynamics and produce raw outputs]:::lightpurple
DSEM[<b>Dynamic Structural Equation Models</b><br/><br/> -<i>External System</i>-<br/><br/> Fit dynamic structural equation models]:::lightpurple
Copy link
Contributor

Choose a reason for hiding this comment

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

Does mermaid have the ability to provide a hyper link to the DSEM repository with the text here?

direction TB
EM[<b>Ecosystem Models</b><br/><br/> -<i>External System</i>-<br/><br/> Simulate ecosystem dynamics and produce raw outputs]:::lightpurple
DSEM[<b>Dynamic Structural Equation Models</b><br/><br/> -<i>External System</i>-<br/><br/> Fit dynamic structural equation models]:::lightpurple
SA[<b>Stock Assessment Models</b><br/><br/> -<i>External System</i>-<br/><br/> Analyze fisheries and environmental impacts on fish stocks]:::lightpurple
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we list example models here like FIMS with a hyperlink instead of "External System"?

%% Define ecosystem simulation toolkit
subgraph ""
direction LR
EcosystemData[<b>ecosystemdata</b><br/><br/> -<i>R package</i>-<br/><br/> -Standardizes model output<br/>-Provides functions to connect with external tools to perform time series analysis]:::lightgreen
Copy link
Contributor

Choose a reason for hiding this comment

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

I do not quite understand what you are trying to imply with "Provides functions to connect with external tools to perform time series analysis"

subgraph ""
direction LR
EcosystemData[<b>ecosystemdata</b><br/><br/> -<i>R package</i>-<br/><br/> -Standardizes model output<br/>-Provides functions to connect with external tools to perform time series analysis]:::lightgreen
EcosystemOM[<b>ecosystemom</b><br/><br/> -<i>R package</i>-<br/><br/> Samples observations from ecosystemdata for stock assessment testing]:::lightgreen
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
EcosystemOM[<b>ecosystemom</b><br/><br/> -<i>R package</i>-<br/><br/> Samples observations from ecosystemdata for stock assessment testing]:::lightgreen
EcosystemOM[<b>ecosystemom</b><br/><br/> -<i>R package</i>-<br/><br/> Samples observations from ecosystemdata to fit to stock assessments]:::lightgreen

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants