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

Add face_sw_direct_flux_dn only for AllSkyRadiationWithClearSkyDiagnostics #3339

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Sbozzolo
Copy link
Member

@Sbozzolo Sbozzolo commented Sep 25, 2024

face_sw_direct_flux_dn is used only in
AllSkyRadiationWithClearSkyDiagnostics in this block:

NVTX.@annotate function update_sw_fluxes!(
    ::AllSkyRadiationWithClearSkyDiagnostics,
    model,
)
    RRTMGP.RTESolver.solve_sw!(
        model.sw_solver,
        model.as,
        model.lookups.lookup_sw,
        nothing,
        model.lookups.lookup_sw_aero,
    )
    parent(model.face_clear_sw_flux_up) .= parent(model.face_sw_flux_up)
    parent(model.face_clear_sw_flux_dn) .= parent(model.face_sw_flux_dn)
    parent(model.face_clear_sw_direct_flux_dn) .=
        parent(model.face_sw_direct_flux_dn)
    parent(model.face_clear_sw_flux) .= parent(model.face_sw_flux)
    RRTMGP.RTESolver.solve_sw!(
        model.sw_solver,
        model.as,
        model.lookups.lookup_sw,
        model.lookups.lookup_sw_cld,
        model.lookups.lookup_sw_aero,
    )
end

It is not used by other modes, so I don't add it

…stics

`face_sw_direct_flux_dn` is used only in
`AllSkyRadiationWithClearSkyDiagnostics` in this block:
```
NVTX.@Annotate function update_sw_fluxes!(
    ::AllSkyRadiationWithClearSkyDiagnostics,
    model,
)
    RRTMGP.RTESolver.solve_sw!(
        model.sw_solver,
        model.as,
        model.lookups.lookup_sw,
        nothing,
        model.lookups.lookup_sw_aero,
    )
    parent(model.face_clear_sw_flux_up) .= parent(model.face_sw_flux_up)
    parent(model.face_clear_sw_flux_dn) .= parent(model.face_sw_flux_dn)
    parent(model.face_clear_sw_direct_flux_dn) .=
        parent(model.face_sw_direct_flux_dn)
    parent(model.face_clear_sw_flux) .= parent(model.face_sw_flux)
    RRTMGP.RTESolver.solve_sw!(
        model.sw_solver,
        model.as,
        model.lookups.lookup_sw,
        model.lookups.lookup_sw_cld,
        model.lookups.lookup_sw_aero,
    )
end
```

It is not used by other modes, so I don't add it
@szy21
Copy link
Member

szy21 commented Sep 25, 2024

Actually, we should have it in other radiation modes as well. Right now for other radiation modes, p.radiation.rrtmgp_model.model.face_sw_direct_flux_dn should have the correct values (hopefully). We don't use it right now but the land may want to access it at some point. We just have it explicitly used for AllSkyRadiationWithClearSkyDiagnostics because we are assigning it to face_clear_sw_direct_flux_dn when calling radiation without clouds.

@Sbozzolo
Copy link
Member Author

Actually, we should have it in other radiation modes as well. Right now for other radiation modes, p.radiation.rrtmgp_model.model.face_sw_direct_flux_dn should have the correct values (hopefully). We don't use it right now but the land may want to access it at some point. We just have it explicitly used for AllSkyRadiationWithClearSkyDiagnostics because we are assigning it to face_clear_sw_direct_flux_dn when calling radiation without clouds.

Where is this set/updated? There's no other reference to it besides that function

@szy21
Copy link
Member

szy21 commented Sep 25, 2024

Where is this set/updated? There's no other reference to it besides that function

If I understand the interface correctly, here we are setting face_sw_direct_flux_dn to be a view of flux_sw. flux_dn_dir (and we set many other things here as well). So after we call e.g. RRTMGP.RTESolver.solve_sw! where flux_sw.flux_dn_dir is updated, face_sw_direct_flux_dn will be updated.

@sriharshakandala
Copy link
Member

sriharshakandala commented Sep 25, 2024

Actually, we should have it in other radiation modes as well. Right now for other radiation modes, p.radiation.rrtmgp_model.model.face_sw_direct_flux_dn should have the correct values (hopefully). We don't use it right now but the land may want to access it at some point. We just have it explicitly used for AllSkyRadiationWithClearSkyDiagnostics because we are assigning it to face_clear_sw_direct_flux_dn when calling radiation without clouds.

I believe, based on our previous discussion, only the value of flux_dn_dir at surface is needed. Currently, the value at the surface should be correct but we do not guarantee that it is correct/updated at all levels!

@Sbozzolo
Copy link
Member Author

Sbozzolo commented Oct 3, 2024

Actually, we should have it in other radiation modes as well. Right now for other radiation modes, p.radiation.rrtmgp_model.model.face_sw_direct_flux_dn should have the correct values (hopefully). We don't use it right now but the land may want to access it at some point. We just have it explicitly used for AllSkyRadiationWithClearSkyDiagnostics because we are assigning it to face_clear_sw_direct_flux_dn when calling radiation without clouds.

I believe, based on our previous discussion, only the value of flux_dn_dir at surface is needed. Currently, the value at the surface should be correct but we do not guarantee that it is correct/updated at all levels!

This is what I see

julia> simulation.integrator.p.radiation.rrtmgp_model.face_sw_direct_flux_dn
11×3456 view(::Matrix{Float32}, 1:11, :) with eltype Float32:
 NaN  NaN  NaN  NaN  NaN  NaN  NaN  NaN  NaN  …  NaN  NaN  NaN  NaN  NaN  NaN  NaN  NaN
 NaN  NaN  NaN  NaN  NaN  NaN  NaN  NaN  NaN     NaN  NaN  NaN  NaN  NaN  NaN  NaN  NaN
 NaN  NaN  NaN  NaN  NaN  NaN  NaN  NaN  NaN     NaN  NaN  NaN  NaN  NaN  NaN  NaN  NaN
 NaN  NaN  NaN  NaN  NaN  NaN  NaN  NaN  NaN     NaN  NaN  NaN  NaN  NaN  NaN  NaN  NaN
 NaN  NaN  NaN  NaN  NaN  NaN  NaN  NaN  NaN     NaN  NaN  NaN  NaN  NaN  NaN  NaN  NaN
 NaN  NaN  NaN  NaN  NaN  NaN  NaN  NaN  NaN  …  NaN  NaN  NaN  NaN  NaN  NaN  NaN  NaN
 NaN  NaN  NaN  NaN  NaN  NaN  NaN  NaN  NaN     NaN  NaN  NaN  NaN  NaN  NaN  NaN  NaN
 NaN  NaN  NaN  NaN  NaN  NaN  NaN  NaN  NaN     NaN  NaN  NaN  NaN  NaN  NaN  NaN  NaN
 NaN  NaN  NaN  NaN  NaN  NaN  NaN  NaN  NaN     NaN  NaN  NaN  NaN  NaN  NaN  NaN  NaN
 NaN  NaN  NaN  NaN  NaN  NaN  NaN  NaN  NaN     NaN  NaN  NaN  NaN  NaN  NaN  NaN  NaN
 NaN  NaN  NaN  NaN  NaN  NaN  NaN  NaN  NaN  …  NaN  NaN  NaN  NaN  NaN  NaN  NaN  NaN

For a allsky case. Is this correct?

@szy21
Copy link
Member

szy21 commented Oct 4, 2024

I think flux_dn_dir is allocated in FluxSW struct but never gets updated in RRTMGP except for the first level here, and that's maybe why it's all NaNs in atmos. But I thought the first level would have correct values?

@sriharshakandala
Copy link
Member

I think flux_dn_dir is allocated in FluxSW struct but never gets updated in RRTMGP except for the first level here, and that's maybe why it's all NaNs in atmos. But I thought the first level would have correct values?

The value at the first level is set here: https://github.com/CliMA/RRTMGP.jl/blob/main/src/rte/shortwave2stream.jl#L233 for the two stream shortwave solver!
Per our earlier discussions, my understanding is that it was only needed at the surface level! I was about to resize the array to only have one level but held off on that change since it would be a breaking change!

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.

3 participants