Local datasets #87
Local datasets #87
Conversation
Persisting the current graph to a yaml file Open comments from #58 will be adressed in separate pull request.
- Dataset is the new base class - DerivedDataset and FullDataset inherit from Dataset
Inheritance structure for Datasets
Reuse Preset::Scaling for DerivedDataset
…_resolved Resolving feedback from PR #58
Also create implementations of Exporter: - FullExporter (corresponds to old Exporter) - EssentialExporter (corresponds to old GraphExporter) Ref https://github.com/quintel/internal/issues/5#issuecomment-265103928 Ref https://github.com/quintel/internal/issues/5#issuecomment-265120551
Local datasets exporter inheritance
- Avoid using send() - Merge in new_attributes after scaled_attributes
Local datasets scaling
Conflicts: lib/atlas/scaler.rb
Local datasets set new ID
Presets should not yet be validated for the fact if they're explictly setting all the shares. Discussed here: https://github.com/quintel/atlas/pull/86\#issuecomment-275112404
As review by @ploh
…-loose-class Create a separate class for an initializer input
Codecov Report@@ Coverage Diff @@
## master #87 +/- ##
=========================================
+ Coverage 92.11% 93.52% +1.4%
=========================================
Files 53 69 +16
Lines 1458 1729 +271
=========================================
+ Hits 1343 1617 +274
+ Misses 115 112 -3
Continue to review full report at Codecov.
|
|
@antw can this be merged? 👼 |
|
|
||
| validates :scaling, presence: true | ||
|
|
||
| validate :base_dataset_exists |
There was a problem hiding this comment.
Could these methods be named "validate_something" for consistency with existing validation methods in Edge, Input, Node, etc. It helps to differentiate validation methods from "normal" methods.
| ####### | ||
| def graph | ||
| @graph ||= | ||
| precomputed_graph? ? dataset.graph : GraphBuilder.build |
There was a problem hiding this comment.
This doesn't need to be split across two lines; the whole thing would be < 80 characters.
|
|
||
| private | ||
|
|
||
| # ActiveDocument. |
There was a problem hiding this comment.
What does "ActiveDocument" mean here? I suspect copy/paste from here?
| @@ -0,0 +1,69 @@ | |||
| module Atlas | |||
| class Scaler | |||
There was a problem hiding this comment.
Could do with some documentation.
| value: value, | ||
| base_value: base_value, | ||
| area_attribute: 'number_of_residences', | ||
| }, |
There was a problem hiding this comment.
Indentation and trailing commas:
scaling: {
value: value,
base_value: base_value,
area_attribute: 'number_of_residences'
}| result[attr] = Util::round_computation_errors(value * @scaling_factor) | ||
| end | ||
| end | ||
| result |
There was a problem hiding this comment.
Replace with each_with_object?
def scale
SCALEABLE_AREA_ATTRS.each_with_object({}) do |attr, data|
if value = @base_dataset[attr]
data[attr] = Util::round_computation_errors(value * @scaling_factor)
end
end
endThere was a problem hiding this comment.
Personally, I still do not really like each_with_object. My version here looks more readable to me (except map instead each, that is a mistake of course). But I am fine either way
| @@ -0,0 +1,63 @@ | |||
| module Atlas | |||
| class Dataset::DerivedDataset < Dataset | |||
There was a problem hiding this comment.
How would you guys feel about naming these two new classes simply "Derived" and "Full"? "Dataset" already appears in the module tree, and it would shorten the .ad file names ("datasets/nl/nl.full.ad" instead of "datasets/nl/nl.full_dataset.ad").
It would also be more consistent with Node subclasses (although I think consistency is not super-important in this case).
There was a problem hiding this comment.
Sounds good to me, what do you think @ploh?
There was a problem hiding this comment.
Yes! Very much in favour of this!
The current names date back to when we did not have a proper class hierarchy for this
There was a problem hiding this comment.
Shall I pick this up? -> #88
| @@ -0,0 +1,72 @@ | |||
| module Atlas | |||
| class GraphDeserializer | |||
| def self.build(graph_hash) | |||
There was a problem hiding this comment.
-
If
buildis the only public way to use this class (i.e. you don't intend for third-party code to ever runnew(...).build_graph), you might want to addprivate_class_method :new: this will enforce that users of this class must usecall.This also applies to AreaAttributesScaler and TimeCurveScaler.
-
A little documentation describing what the
graph_hashparameter is – and what the method returns – would be a nice addition.
There was a problem hiding this comment.
👍
(Sorry for these emoji only remarks. The mobile version does not seem to allow the direct emoji reactions)
- Renaming validation methods for derived dataset class - Removing line of comment - Removing newline
| class Scaler | ||
| UNSCALED_ETENGINE_DATA_FILES = %w( fce load_profiles network ).freeze | ||
|
|
||
| def initialize(base_dataset_key, derived_dataset_name, number_of_residences, base_value = nil) |
There was a problem hiding this comment.
@ploh Why is base_value used as the 4th optional argument? It won't be picked up by rake scale as far as I know.
There was a problem hiding this comment.
But by the scenario migration rake task in etengine that i am going to push, hopefully tomorrow
+ Add documentation about the
|
@antw I fixed most comments. Some of them seem like nice to have's and could maybe be put into issues I think. If there's time we'll fix them but it would be nice if this can be merged today (no pressure). |
All things related to local datasets for Atlas
Contains part of #88