-
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
Add architecture design document #41
Conversation
@@ -0,0 +1,9 @@ | |||
# Architecture and Design |
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.
Why not "Architecture decision records"? This would make it easier to find given the Scipp docs and it allows us to add more if need be.
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 had considered adding this as a subsection. I thought we might have other documentation here as well, not just decision records?
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.
ok
transmission_monitor: sc.DataArray, | ||
""" |
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.
The docstring has slipped in between the arguments and return annotation.
However, instead of the user having to pass these to functions, we use dependency injection to provide them to the functions. | ||
In essence this will build a workflow's task graph. | ||
|
||
From the [Guice documentation](https://github.com/google/guice/wiki/MentalModel#injection) (Guice is a dependency injection framework for Java): |
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.
Move to the Dependency injection section.
incident_monitor: IncidentMonitor, | ||
transmission_monitor: TransmissionMonitor, | ||
) -> TransmissionFraction: | ||
return transmission_monitor / incident_monitor |
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.
return transmission_monitor / incident_monitor | |
return TransmissionFraction(transmission_monitor / incident_monitor) |
It's unfortunately verbose but should be part of the design discussion.
Generally, the user must provide configuration parameters to a workflow. | ||
In many cases there are defaults that can be used. | ||
In either case, these parameters must be associated with the correct step in the workflow. | ||
This is complicated by the non-linear nature of the workflow. |
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.
This is complicated by the non-linear nature of the workflow. | |
This gets complicated by the non-linear nature of the workflow. |
?
## Notes | ||
|
||
- Earlier versions of this design document included a detailed discussion on how to handle nested workflows using child-injectors. | ||
During initial implementation efforts this idea was discarded, as it was found to be too complicated and not necessary. |
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.
Duplicate of l. 181f.
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.
Um, what is duplicate?
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.
Maybe not a full duplicate. But the other section also mentions that nested workflows were considered but not implemented.
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.
Isn't the section talking about nested parameters, as opposed to nested workflows?
Fixes scipp/ess#157.