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

Interpolation behavior #643

Closed
bpbond opened this issue Oct 28, 2022 · 5 comments · Fixed by #646
Closed

Interpolation behavior #643

bpbond opened this issue Oct 28, 2022 · 5 comments · Fixed by #646
Assignees
Labels

Comments

@bpbond
Copy link
Member

bpbond commented Oct 28, 2022

Current behavior

Hector time series (input data that are time-variant) can be marked as interpolation allowed or not allowed. The land-use emissions time series allows interpolation, because if the solver stops in 1800.5, we want it to 'see' an annualized LUC value that's halfway between that of 1800 and 1801.

But for a single-year pulse test, as in @dawnlwoodard 's in #639 , this produces very undesirable behavior:

Fri Oct 28 03:05:33 2022:NOTICE:run: 1799
Fri Oct 28 03:05:33 2022:NOTICE:run: 1800
-----------------------
1800 yf = 1
	luc_e = 10
	veg_c = 558.996 Pg C
	global luc_fva_biome_flux = 2.11675 Pg C
	global veg_c = 556.879 Pg C after luc

Fri Oct 28 03:05:33 2022:NOTICE:run: 1801
-----------------------
1800.5 yf = 0.5
	luc_e = 5
	veg_c = 557.938 Pg C
	global luc_fva_biome_flux = 0.529194 Pg C
	global veg_c = 557.408 Pg C after luc
	
-----------------------
1800.75 yf = 0.25
	luc_e = 2.5
	veg_c = 557.158 Pg C
	global luc_fva_biome_flux = 0.1323 Pg C
	global veg_c = 557.025 Pg C after luc

Fri Oct 28 03:05:33 2022:NOTICE:run: 1802
Fri Oct 28 03:05:33 2022:NOTICE:run: 1803

Whoops! When the solver hit 1800, the land emits 10 Pg C of LUC emissions, which is what we wanted...but then another 5 (halfway through the year, so a value halfway between 10 and 0) gets debited in 1800.5 and another 2.5 in 1800.75.

We've ended up with a total LUC pulse of 17.5 when the input files specified only 10.

@bpbond bpbond added the bug label Oct 28, 2022
@bpbond bpbond self-assigned this Oct 28, 2022
@bpbond
Copy link
Member Author

bpbond commented Oct 28, 2022

New behavior

In b6cf6cb we change things so that LUC, FFI, and DACCS are retrieved from their respective time series only once, at the beginning of the timestep in slowparameval(). These private values — current_luc_e, current_luc_u, etc. — are then referenced throughout calcderivs() and stashCValues().

Fri Oct 28 03:53:53 2022:NOTICE:run: 1799
Fri Oct 28 03:53:53 2022:NOTICE:run: 1800
Fri Oct 28 03:53:53 2022:NOTICE:run: 1801
-----------------------
1801 yf = 1
	luc_e = 10
	veg_c = 558.996 Pg C
	global luc_fva_biome_flux = 2.11675 Pg C
	global veg_c = 556.879 Pg C after luc

Fri Oct 28 03:53:53 2022:NOTICE:run: 1802
Fri Oct 28 03:53:53 2022:NOTICE:run: 1803
Fri Oct 28 03:53:53 2022:NOTICE:run: 1804

👍

This also reduces the code size, which is a nice bonus.

bpbond added a commit that referenced this issue Oct 28, 2022
Remove old `ffi()`, etc. functions; see #643
@bpbond
Copy link
Member Author

bpbond commented Oct 28, 2022

@dawnlwoodard

pulse

@bpbond
Copy link
Member Author

bpbond commented Oct 28, 2022

I am so enamored of this LUC pulse test — it's a stringent test of model behavior — that adding it as a permanent test.

@dawnlwoodard
Copy link
Collaborator

@bpbond OMG THIS EXPLAINS SO MUCH. It never made sense to me that Hector was regrowing more carbon than it lost in my pulse experiments, because that isn't how returning to equilibrium should work. But this totally explains it!! So glad you figured it out and fixed it!

@dawnlwoodard
Copy link
Collaborator

Also, that is SO satisfying to see vegetation just drop to a new equilibrium.

bpbond added a commit that referenced this issue Oct 31, 2022
Remove old `ffi()`, etc. functions; see #643
@bpbond bpbond linked a pull request Oct 31, 2022 that will close this issue
bpbond added a commit that referenced this issue Nov 1, 2022
* Set current LUC, FFI, DACCS once at beginning of timestep
* Remove old `ffi()`, etc. functions; see #643
* Disallow interpolation of emissions/uptake time series
* Add LUC pulse test (disabling post-pulse test)
* Update 1745-1749 constraints to non-zero values
* Re-enable old-new test (and rename for consistency)
@bpbond bpbond closed this as completed Nov 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
2 participants