-
Notifications
You must be signed in to change notification settings - Fork 7
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
Initializer inputs #894
Initializer inputs #894
Conversation
2ee6cbc
to
8a2fc9e
Compare
5013ee3
to
7069430
Compare
7069430
to
5c49a94
Compare
@grdw I pushed my suggestions for this. Please review, further adapt if necessary and merge. Thx. |
|
||
def apply_initializer_inputs(graph = nil) | ||
unless graph |
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 a bit of a weird solution to be honest, why cramp everyting in a single method?
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.
Yeah, it seemed like a cool idea earlier, but now at second glance it looks weird!
init. | ||
map { |key, val| [Input.fetch(key), val] }. | ||
sort_by { |input, _| input.priority }. | ||
reverse |
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 a bit of a fan of splitting these things up in several methods like:
def initializer_inputs
decorated_inputs.sort_by { |input, _| input.priority }.reverse
end
private
def decorated_inputs
(area.init || {}).map do |key, val|
[Input.fetch(key), val]
end
end
... or something like that.
Rationale: Qernel::Dataset is really only a data container
d9c7d97
to
02ee402
Compare
This is the most basic implementation of the initializer inputs.
Related to: quintel/atlas#81