Skip to content

0723 agricolture #18

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

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

0723 agricolture #18

wants to merge 5 commits into from

Conversation

mzecc
Copy link
Collaborator

@mzecc mzecc commented Jun 26, 2025

Description

AWB instances reclassified back into Agriculture

Checklist

Please confirm that this pull request has done the following:

  • Tests added
  • Documentation added (where applicable)
  • Changelog item added to changelog/

Copy link
Collaborator

@jkikstra jkikstra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @mzecc changes look like they make sense.

Two questions:

  • did you also manually check that the aggregation works (numbers add up fine?) with an example?
  • why is the uv.lock changing?

Before approving, I will also still try to run the tests and then I suppose we're good to go.

@jkikstra
Copy link
Collaborator

* did you also manually check that the aggregation works (numbers add up fine?) with an example?

For https://github.com/openscm/gcages/blob/main/tests/regression/cmip7-scenariomip/test-data/salted-202504-scenariomip-input.csv, the pre-processing (like here) using this notebook (placed in gcages root folder): https://github.com/jkikstra/gcages/blob/pr/18-jsk/test_basic_aggregation.py, I can confirm that the aggregation works as expected.

@jkikstra
Copy link
Collaborator

But uv run pytest tests is not happy:

(base) C:\Users\kikstra\Documents\GitHub\gcages>uv run pytest tests
======================================================================================== test session starts =========================================================================================
platform win32 -- Python 3.11.5, pytest-8.3.4, pluggy-1.5.0
rootdir: C:\Users\kikstra\Documents\GitHub\gcages
configfile: pyproject.toml
collected 1331 items / 1 error / 2 skipped

=============================================================================================== ERRORS ===============================================================================================
_____________________ ERROR collecting tests/integration/cmip7-scenariomip/pre-processing/reaggregation/test_integration_cmip7_scenariomip_pre_processing_reaggregation_basic.py _____________________
tests\integration\cmip7-scenariomip\pre-processing\reaggregation\test_integration_cmip7_scenariomip_pre_processing_reaggregation_basic.py:854: in <module>
    dict(atol=Q(1.0, "MtCO2 / yr")),
E   TypeError: 'NoneType' object is not callable
====================================================================================== short test summary info =======================================================================================
ERROR tests/integration/cmip7-scenariomip/pre-processing/reaggregation/test_integration_cmip7_scenariomip_pre_processing_reaggregation_basic.py - TypeError: 'NoneType' object is not callable
!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!! Interrupted: 1 error during collection !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!
==================================================================================== 2 skipped, 1 error in 0.68s =====================================================================================

@mzecc mzecc closed this Jun 30, 2025
@mzecc mzecc reopened this Jun 30, 2025
@mzecc
Copy link
Collaborator Author

mzecc commented Jul 1, 2025

I have updated the salted (test) file we checked. Everything looks consistent to me.

@mzecc
Copy link
Collaborator Author

mzecc commented Jul 2, 2025

Made the changes to handle the fact that AFOLU emissions will not be provided as gridded emissions.
The logic is that co2 data frame is handled separately and "concat" to the region_sector_df_gridding filling with zeros the AFOLU-sectors that are currently expecting co2 values for gridding.
Tests are currently failing but I guess it is expected.

@jkikstra
Copy link
Collaborator

jkikstra commented Jul 2, 2025

Made the changes to handle the fact that AFOLU emissions will not be provided as gridded emissions. The logic is that co2 data frame is handled separately and "concat" to the region_sector_df_gridding filling with zeros the AFOLU-sectors that are currently expecting co2 values for gridding. Tests are currently failing but I guess it is expected.

Thanks @mzecc. I had thought to maybe work in 3 PRs to keep the chunks easy-to-review (test, check, run, document):

  1. AWB -> Agriculture (for some variables)
  2. CO2|AFOLU
  3. new CDR sectors

@mzecc
Copy link
Collaborator Author

mzecc commented Jul 2, 2025

Yes, I agree. Thanks @jkikstra. I'll start a new PR. This one seems complete to me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants