Skip to content
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

Feature/flux model v1 4 0 run #248

Merged
merged 10 commits into from
Dec 18, 2024
Merged

Conversation

dagibbs22
Copy link
Member

Pull request checklist

Please check if your PR fulfills the following requirements:

  • [] Make sure you are requesting to pull a topic/feature/bugfix branch (right side). Don't request your master!
  • [] Make sure you are making a pull request against the develop branch (left side). Also you should start your branch off our develop.
  • Check the commit's or even all commits' message styles matches our requested structure.
  • Check your code additions will fail neither code linting checks nor unit test.

Pull request type

Please check the type of change your PR introduces:

  • Bugfix
  • Feature
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • Documentation content changes
  • Other (please describe):

What is the current behavior?

  1. carbonflux package used forest flux model v1.3.2.
  2. carbonflux package output emissions for CO2, non-CO2, and all gases combined.
  3. carbonflux package used 10-km drivers map

What is the new behavior?

  1. carbonflux package uses forest flux model v1.4.0
  2. carbonflux package outputs emissions for CO2, CH4, N2O, non-CO2 gases combined, and all gases combined.
  3. carbonflux package uses 1-km driver map

Does this introduce a breaking change?

  • Yes
  • No

Other information

Uses GDAL 3.1.2. Was not able to run this locally and had to test GMB and IDN14.13 in EMR through the sbt shell.

…n-CO2 emissions. It still outputs non-CO2 emissions, as well as CO2, CH4, N2O, and all gases combined. Had to add new layers for Ch4 and N2O emissions for biomass_soil and soil_only.
…ked ArcPy output) but soil_only emissions are strange. Some rows of soil_only emissions have CH4 and N2O emissions but no CH4 or N2O for biomass_soil, nor the non-CO2 total for soil_only. I can't figure out why. Mel has pretty much ruled out problems with the input rasters.
…nged them to soil_only. It was the only way I could think of to get clean soil_only emissions code; clearly something was wrong with the soil_only code before but I couldn't identify it. This produced outputs in the correct columns for GMB. Running for IDN14.13, then need to check against ArcPy QC script results.
Copy link
Collaborator

@danscales danscales left a comment

Choose a reason for hiding this comment

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

Generally, it looks good.

But, did you notice that the tests failed? That's because we now have two tests for annualupdate_minimal. Does it make sense that the results for annualupdate_minimal changed? I see that it use netFluxCo2 and grossEmissionsCo2e{NonCo2,CO2Only}, etc. in its analysis, so it probably does, since you updated versions of some of those datasets, but you should probably double-check. If it seems fine, then I can show you how to update the test results so the annualupdate_minimal tests pass again.

@danscales danscales self-requested a review December 18, 2024 20:14
Copy link
Collaborator

@danscales danscales left a comment

Choose a reason for hiding this comment

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

Looks good. I'll fix the tests.

@dagibbs22 dagibbs22 merged commit 34315bc into master Dec 18, 2024
2 of 3 checks passed
@dagibbs22 dagibbs22 deleted the feature/flux_model_v1_4_0_run branch December 18, 2024 20:16
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