-
Notifications
You must be signed in to change notification settings - Fork 0
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
Just for Harvesting: Standard charts #20
base: master
Are you sure you want to change the base?
Conversation
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.
Only reviewed projection_charts.clj, but I've got a good start on getting a handle on what's going on.
Looks like a really good body of work and am keen to test it out.
A few things I'd like clarified, a few discussion points that we might consider and maybe one what I assume is a typo
(map (fn [x] [(:setting x) (:setting x)])) | ||
census) | ||
needs-lookup (into {} | ||
(map (fn [x] [(:setting x) (:setting x)])) |
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.
not :need
?
needs-lookup (into {} | ||
(map (fn [x] [(:setting x) (:setting x)])) | ||
census) | ||
output-ay (future |
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.
okay, so this means the code will start processing output-ay
and move on to output-count
without waiting for output-ay
to finish, as it's not dependent
(vis.oay/output-ay (str output-dir "/" vis.oay/output-ay-file))))) | ||
output-count (future | ||
(vis.ocount/chart | ||
"2020 Baseline" |
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 probably wants to be an argument somehow, rather than a fixed (magic) string. And I can see it elsewhere but won't add more comments
general-population (future (vis.gp/charts gen-pop-data))] | ||
|
||
(a/thread | ||
;; (run! (partial wsc/save-chart-by-title (str output-dir "/charts/ay-")) @output-ay) |
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.
So is this because you don't want the "raw" pngs outside of the spreadsheet? Maybe it should be an option or just removed? Or even another function that can do it instead?
setting-titles-and-sets) | ||
general-population (future (vis.gp/charts gen-pop-data))] | ||
|
||
(a/thread |
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.
So this is using thread
because it is dependent on output-ay
but you still want it to run on a new, unsued thread correct? And I guess all of this parallelisation (is that even what it actually is?) speed is only achieved based on how many cores you provide to the process. Does Clojure just make use of how ever many you provide it. I know some other languages you still have to make it much clearer how you want the resources to be used.
For validation and projections.
Currently using old structure of code and getting to the output in two different ways. It would be good to choose one of them and I think the flow of the validation code is a bit better.