Conversation
|
Note that This has still to with the |
|
@kndehaan I have pushed a fix for the biogenic output content carriers now |
noracato
left a comment
There was a problem hiding this comment.
I'm worried that the UPDATE part does not have enough validation.
Can you please add tests that show updating nonexistent keys will not work? I have a feeling we will need the DatasetAttributes module after all (it is still on the emissions-gql branch)
| # See Qernel::Dataset#assign_dataset_attributes to understand what's going on: | ||
| call_on_each_qernel_object(:assign_dataset_attributes) | ||
| # Manually assign emissions hash (not a DatasetAttributes object) | ||
| @emissions = dataset&.data&.[](:emissions) || {} |
There was a problem hiding this comment.
Can you explain once more why not to use DatasetAttributes? Like this there is no validation on what users can set as an emissions attribute.
There was a problem hiding this comment.
Good point, I was not really looking at UPDATE yet, so I thought to keep it as light as possible for read.
However, you are right that it's cleaner to just use the DatasetAttributes approach for consistency and validation.
I re-instated the DatasetAttributes approach and added more tests to cover the case of nonexistent keys etc. I expect more spec will be needed based on the changes for 1990 but I think it's best to process that separately.
|
Adding myself as reviewer to check the descriptions for the methods (it should be understandable and correct from a modeller's perspective as well). |
| # Total CO2 utilised (consumed as feedstock) at this node. | ||
| # | ||
| # Currently returns only fossil utilisation, as biogenic utilisation is always 0. | ||
| # | ||
| # @return [Float, nil] Total CO2 utilised in kg, or nil if node is not in emissions group | ||
| def direct_co2_input_utilisation | ||
| with_emissions_node do | ||
| direct_co2_input_utilisation_fossil | ||
| # Potentially in the future: + direct_co2_input_utilisation_biogenic (currently 0) | ||
| end | ||
| end |
There was a problem hiding this comment.
I think we decided to remove this method for now, as we currently don't use it right now, right? @louispt1
There was a problem hiding this comment.
Yes this method snuck back in with the csv serialiser merge - I will remove it
| VALID_GHG_TYPES = %w[co2 other_ghg n2o ch4 hfc pfc sf6 nf3].freeze | ||
| VALID_GHG_PATTERN = /^(#{VALID_GHG_TYPES.join('|')})(_\d{4})?$/.freeze |
There was a problem hiding this comment.
Sorry to comment again, but let's not use constants with carriers and patterns. These keys have nothing to do with engine validations, they are part of the dataset pipeline IMO - or at least ETSource. The engine should not carry this knowledge.
There was a problem hiding this comment.
No need to apologise, you're spot on. I pushed a change removing the validation.
Edit:
However now I need to figure out how to handle validation a little smarter - still working on it :)
There was a problem hiding this comment.
@noracato what do you think about this approach?
noracato
left a comment
There was a problem hiding this comment.
Sorry to keep nitpicking. There is still logic present in the module that I feel over complicates it.
| # For setters, check if the emission key exists in the dataset | ||
| if method_name.to_s.end_with?('=') | ||
| data_key = scoped_method(method_name.to_s.sub(/=$/, '')) | ||
| @emissions.dataset_has_key?(data_key) | ||
| else | ||
| # Getters always respond (may return nil if key doesn't exist) | ||
| true | ||
| end |
There was a problem hiding this comment.
| # For setters, check if the emission key exists in the dataset | |
| if method_name.to_s.end_with?('=') | |
| data_key = scoped_method(method_name.to_s.sub(/=$/, '')) | |
| @emissions.dataset_has_key?(data_key) | |
| else | |
| # Getters always respond (may return nil if key doesn't exist) | |
| true | |
| end | |
| # Getters always respond (may return nil if key doesn't exist) | |
| return true unless method_name.to_s.end_with?('=') | |
| @emissions.dataset_has_key?( | |
| scoped_method(method_name.to_s.sub(/=$/, '')) | |
| ) |
General approach is good. This is a bit pythonesque ;)
There was a problem hiding this comment.
Actually, what was wrong with:
def respond_to_missing?(method_name, include_private = false)
data_key = scoped_method(method_name).split('=').first
@emissions.respond_to?(data_key) || super
end| # Check both string and symbol keys since datasets may use either | ||
| dataset_attributes.key?(key.to_s) || dataset_attributes.key?(key.to_sym) |
There was a problem hiding this comment.
I don't feel this should be handled here?
There was a problem hiding this comment.
I found a critical bug that needs to be fixed. It concerns using the EMISSIONS() function for setting demand on dataset value molecule nodes. It works for the nl datasets (ETDataset datasets), but I think it does not work as it should for ETLocal datasets.
Example blank nl2023 scenario, querying the three things below gives the expected, desired other ghg emissions (don't mind the differences in decimals):
EACH(
EMISSIONS(agriculture_non_specified, non_energetic, other_ghg),
MV(agriculture_non_specified_non_energetic_other_ghg, demand),
MV(agriculture_non_specified_non_energetic_other_ghg, direct_reporting_emissions_other_ghg_emissions)
)
[
17,888.11512,
17,888,115,120.0,
17,888,115,120.0,
]
This is however not the case for for example blank NO_norway scenario (and other countries as well):
EACH(
EMISSIONS(agriculture_non_specified, non_energetic, other_ghg),
MV(agriculture_non_specified_non_energetic_other_ghg, demand),
MV(agriculture_non_specified_non_energetic_other_ghg, direct_reporting_emissions_other_ghg_emissions)
)
[
4,516.46047,
0.0,
0.0,
]
So I think something in reading with ~ demand on the molecule nodes goes wrong when it's a ETLocal dataset (derived dataset).
I know that for energy nodes, the derived dataset first looks if graph_methods is specified. If not, it falls back to what is defined with ~, whereas full datasets directly look at ~ and don't look at graph_methods.
Might it be that for the molecule nodes, something has to be configured for derived datasets to also look at values specified with~?
|
I'll have a look. Gut feeling is that you are right. There might also be a hidden scaling factor involved. |
|
Additionally, if I set a hard-coded value on the molecule node like |
|
I found the culprit. When calculating the initial graph, there is an extra thing applied for Here you can see a comment saying "Derived datasets have no molecule flows." I will try to find out why this was setup like this, and if we can lift it, or if we can create a special case for "floating" nodes. |
|
In the end, working with the emission methods as they are defined now is not the most intuitive: it is a bit too elaborate in practice. How did you experience this @kndehaan? Not saying this is a must have, but still good to flag. More straightforward might be:
Also, a |
|
@mabijkerk let's review your comment in the next increment. I agree that the naming of the methods has not been most intuitive. Also, being able to query the direct reporting CO2 emissions of a node would be useful. |
Dismissing the requested changes as the required solution has been implemented via Atlas, Transformer, ETSource.
…co2_output_production_emissions_fossil and spec
…ce of ccs_capture_rate. Update spec
…thod as direct_co2_input_utilisation_fossil is sufficient
…_utilised and emissions_lulucf_removals checks from spec
…now it covers more cases
…ut co2 content approach
* Expand ConfiguredCSVSerializer with node group expansion, molecule support, and emissions CSV endpoints * Add ghg_carrier method to DirectEmissions/MoleculeEmissions and update test fixture of the direct_emissions_csv * Add ghg_carrier to MoleculeEmissions specs and simplify DirectEmissions specs to use faster mock-based approach * Return symbols from ghg_carrier instead of strings
Co-authored-by: kndehaan <102598197+kndehaan@users.noreply.github.com>
… the emissions.csv file
…gql approach more closely
* Add a validation lib spec for node values per dataset * Add dev and test modes for graph data validation
Summary of changes
Emissions Calculation Framework
ccs_capture_rate) and utilization (viaco2_utilisation_per_mj)Carbon Content Tracing
emissions_skip_crude_oil_mixedge group for forcing weighted mix calculationsGQL & Dataset Integration
Dataset::ImportandEtsource::LoaderSupporting Infrastructure
free_co2_factorfor capture calculations with consistent resultsOther
Note: Atlas reference should be updated once the Atlas
emissionsbranch has been merged.Goes with: