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

Wrong OLR weather value in ACCF contrail #64

Closed
Reyrem opened this issue Jun 15, 2023 · 3 comments
Closed

Wrong OLR weather value in ACCF contrail #64

Reyrem opened this issue Jun 15, 2023 · 3 comments
Assignees
Labels
bug Something isn't working

Comments

@Reyrem
Copy link

Reyrem commented Jun 15, 2023

Description

The contrail accf by day is not using the right weather variable for outgoing longwave radiation. Climaccf is using "top net thermal radiation (ttr)" in J m^-2 from ERA5 single level dataset. It is then divided in their implementation by the time interval (3600s in this case) to convert it to the mean flux on this time interval (W m^-2). Climaccf should therefore be given the value in J m^-2, and not the one in W m^-2.

This is the expected result with Climaccf :
climaccf_contrailACCF

This is the result with Pycontrail accf:
pycontrail_contrailACCF

This is the result with climaccf with the wrong OLR values :
climaccf_contrailACCF_wrong_OLR

We think this is the main difference between the two results.

Details

  • Version: 0.42.0
  • Module: models.accf
  • OS: windows

Steps to Reproduce

  1. Run the accfs with pycontrail
  2. Compare with Climaccf with both the weather variable in J/m-² and in W/m-²
@mlshapiro
Copy link
Contributor

This is super helpful, thanks @Reyrem .

We made the choice to modify the accumulated values on load because it made downstream modeling simpler, but only for the internally built models. I think it was an incorrect choice, so we'll find a way to stop modifying the input meteorology by default.

@trdean1
Copy link
Contributor

trdean1 commented Sep 19, 2023

I just ran across this issue in some comparison work I was doing. A work around to this would be to multiply by 3600*step_size after this line in our wrapper code.

@mlshapiro is there any objection to doing this now and removing it once we change how the met data gets loaded?

@zebengberg
Copy link
Contributor

Fixed in #104

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

4 participants