Add kaya_variables, kaya_factors, and kaya_lmdi methods to the comput… #884

zacharyschmidt · 2024-10-30T04:49:58Z

…e module. Also add the kaya subdirectory that contains the implementation for the kaya methods. (#875)

Please confirm that this PR has done the following:

Tests Added
Documentation Added
Name of contributors Added to AUTHORS.rst
Description in RELEASE_NOTES.md Added

Description of PR

This PR adds three methods to the public api of the compute module: kaya_variables(), kaya_factors(), and kaya_lmdi(). The implementation for these methods is in the new kaya submodule.

…e module. Also add the kaya subdirectory that contains the implementation for the kaya methods. (IAMconsortium#875)

codecov · 2024-11-02T16:58:17Z

Codecov Report

Attention: Patch coverage is 96.14325% with 14 lines in your changes missing coverage. Please review.

Project coverage is 95.1%. Comparing base (ddbb88e) to head (b741dbb).
Report is 33 commits behind head on main.

Files with missing lines	Patch %	Lines
pyam/compute.py	87.2%	7 Missing ⚠️
pyam/kaya/kaya_factors.py	88.8%	4 Missing ⚠️
pyam/kaya/kaya_variables.py	94.9%	3 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##            main    #884     +/-   ##
=======================================
+ Coverage   95.0%   95.1%   +0.1%     
=======================================
  Files         64      74     +10     
  Lines       6134    6579    +445     
=======================================
+ Hits        5828    6260    +432     
- Misses       306     319     +13

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

danielhuppmann · 2024-11-02T17:09:41Z

Thanks for the PR! Looks like the legacy-test is failing is because the unit's are ordered differently

model   scenario region variable unit                        year              
model_a scen_a   World  GNP/P    USD * billion / a / million 2010    NaN  0.006
                                 USD * billion / million / a 2010  0.006    NaN

Will take a look more closely next week, don't worry about the failing test for now, will think about how to avoid that.

danielhuppmann

Took a first look at the PR and made a few comments for improvements, Quite a lot of material here...

It would be helpful if you could remove the LMDI decomposition and add that as a separate PR later.

danielhuppmann · 2024-11-12T05:15:21Z

pyam/compute.py

+        scenarios : iterable of tuples (model, scenario, region)
+            The (model, scenario, region) combinations to be included.


Wondering about two issues with the scenarios filter:

Why not just compute kaya-variables for all data in the IamDataFrame?

If it's necessary to filter, why not allow filter-kwargs and add those below, like?
def kaya_variables(self, append=False, **kwargs) _data = self._df.filter(**kwargs) ...

I'm concerned about adding an argument "scenarios" that is actually a combination of model-scenario-region, that's quite confusing...

danielhuppmann · 2024-11-12T05:18:26Z

pyam/compute.py

@@ -249,6 +251,248 @@ def bias(self, name, method, axis):
        """
        _compute_bias(self._df, name, method, axis)

+    def kaya_variables(self, scenarios, append=False):


There's a method kaya_variables and an imported module with the same name, which then also has a method of that name... Hard to follow the code logic here.

danielhuppmann · 2024-11-12T05:19:59Z

tests/test_feature_kaya_factors.py

+TEST_DF = IamDataFrame(
+    pd.DataFrame(
+        [
+            [input_variable_names.POPULATION, "million", 1000],


From a testing perspective, it would be useful to have the actual values here, not the module-internal translation...

Suggested change

[input_variable_names.POPULATION, "million", 1000],

["Population", "million", 1000],

danielhuppmann · 2024-11-12T05:30:50Z

pyam/kaya/kaya_variables.py

+        kaya_variable_frames.append(_calc_pop(input))
+        kaya_variable_frames.append(_calc_gdp(input))
+        kaya_variable_frames.append(_calc_fe(input))
+        kaya_variable_frames.append(_calc_pe(input))
+        kaya_variable_frames.append(_calc_pe_ff(input))
+        kaya_variable_frames.append(_calc_tfc(input))
+        kaya_variable_frames.append(_calc_nfc(input))


Would be more efficient (and easier to read) to do:

Suggested change

kaya_variable_frames.append(_calc_pop(input))

kaya_variable_frames.append(_calc_gdp(input))

kaya_variable_frames.append(_calc_fe(input))

kaya_variable_frames.append(_calc_pe(input))

kaya_variable_frames.append(_calc_pe_ff(input))

kaya_variable_frames.append(_calc_tfc(input))

kaya_variable_frames.append(_calc_nfc(input))

kaya_variable_frames.append = pyam.concat(

[

_calc_pop(input),

_calc_gdp(input),

_calc_fe(input),

_calc_pe(input),

_calc_pe_ff(input),

_calc_tfc(input),

_calc_nfc(input),

]

)

danielhuppmann · 2024-11-12T05:58:21Z

pyam/kaya/kaya_factors.py

+        )
+        if input.empty:
+            break
+        kaya_factors_frames.append(_calc_gnp_per_p(input))


As in my other comment, this could be simplified using pyam.concat().

danielhuppmann · 2024-11-12T06:01:08Z

pyam/kaya/lmdi_names.py

@@ -0,0 +1,7 @@
+Pop_LMDI = "Population (LMDI)"


Following the current IAMC-variable naming convention, it would be preferably to use square brackets for these identifiers.

Suggested change

Pop_LMDI = "Population (LMDI)"

Pop_LMDI = "Population [LMDI]"

danielhuppmann · 2024-11-12T06:11:05Z

pyam/compute.py

+        """Compute the variables needed to compute Kaya factors
+        for the Kaya Decomposition Analysis.


The first sentence of the docstring should fit in one line, per numpy conventions.

Suggested change

"""Compute the variables needed to compute Kaya factors

for the Kaya Decomposition Analysis.

"""Compute the variables needed to compute Kaya factors

danielhuppmann · 2024-11-12T06:12:03Z

pyam/compute.py

+        """Compute the Kaya factors needed to compute factors
+        for the Kaya Decomposition Analysis.


Suggested change

"""Compute the Kaya factors needed to compute factors

for the Kaya Decomposition Analysis.

"""Compute the factors for the Kaya Decomposition Analysis

danielhuppmann · 2024-11-12T06:14:13Z

pyam/kaya/kaya_factors.py

+        ignore_units=True,
+        append=False,
+    ).rename(unit={"unknown": ""})


Should be possible to directly se the resulting unit.

Suggested change

ignore_units=True,

append=False,

).rename(unit={"unknown": ""})

ignore_units="",

append=False,

)

Add kaya_variables, kaya_factors, and kaya_lmdi methods to the comput…

b741dbb

…e module. Also add the kaya subdirectory that contains the implementation for the kaya methods. (IAMconsortium#875)

danielhuppmann assigned zacharyschmidt Nov 2, 2024

danielhuppmann reviewed Nov 12, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add kaya_variables, kaya_factors, and kaya_lmdi methods to the comput… #884

Add kaya_variables, kaya_factors, and kaya_lmdi methods to the comput… #884

zacharyschmidt commented Oct 30, 2024

codecov bot commented Nov 2, 2024

danielhuppmann commented Nov 2, 2024

danielhuppmann left a comment

danielhuppmann Nov 12, 2024

danielhuppmann Nov 12, 2024

danielhuppmann Nov 12, 2024

danielhuppmann Nov 12, 2024

danielhuppmann Nov 12, 2024

danielhuppmann Nov 12, 2024

danielhuppmann Nov 12, 2024

danielhuppmann Nov 12, 2024

danielhuppmann Nov 12, 2024

		scenarios : iterable of tuples (model, scenario, region)
		The (model, scenario, region) combinations to be included.

	[input_variable_names.POPULATION, "million", 1000],
	["Population", "million", 1000],

-        kaya_variable_frames.append(_calc_pop(input))
-        kaya_variable_frames.append(_calc_gdp(input))
-        kaya_variable_frames.append(_calc_fe(input))
-        kaya_variable_frames.append(_calc_pe(input))
-        kaya_variable_frames.append(_calc_pe_ff(input))
-        kaya_variable_frames.append(_calc_tfc(input))
-        kaya_variable_frames.append(_calc_nfc(input))
+        kaya_variable_frames.append = pyam.concat(
+            [
+                _calc_pop(input),
+                _calc_gdp(input),
+                _calc_fe(input),
+                _calc_pe(input),
+                _calc_pe_ff(input),
+                _calc_tfc(input),
+                _calc_nfc(input),
+            ]
+        )

	Pop_LMDI = "Population (LMDI)"
	Pop_LMDI = "Population [LMDI]"

		"""Compute the variables needed to compute Kaya factors
		for the Kaya Decomposition Analysis.

		"""Compute the Kaya factors needed to compute factors
		for the Kaya Decomposition Analysis.

	"""Compute the Kaya factors needed to compute factors
	for the Kaya Decomposition Analysis.
	"""Compute the factors for the Kaya Decomposition Analysis

Add kaya_variables, kaya_factors, and kaya_lmdi methods to the comput… #884

Are you sure you want to change the base?

Add kaya_variables, kaya_factors, and kaya_lmdi methods to the comput… #884

Conversation

zacharyschmidt commented Oct 30, 2024

Please confirm that this PR has done the following:

Description of PR

codecov bot commented Nov 2, 2024

Codecov Report

danielhuppmann commented Nov 2, 2024

danielhuppmann left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment