Correct accounting for eq_11 term in surface derivatives#931
Conversation
|
I want to raise a small concern just to be sure we are treating and Which is interesting because this means that, And so, I believe we were incorrect yesterday then in our discussion... And the current way And so, for the derivative |
|
Very interesting, thanks, Brent! It took me a bit, but I see your point. If we make the change proposed here, we are, in efect, "double-counting" the For self-consistency with the derivatives, aren't we only simplifying to Eq. 15 in the 3c-case where we set the |
| L_dif_dif, | ||
| ) = RT.calc_RT_quantities(x_RT, geom) | ||
| ) = RT.calc_RT_quantities( | ||
| x_RT, geom, rho_dif_dif=0 # Assume no surface-atmosphere coupling |
There was a problem hiding this comment.
@brentwilder @pgbrodrick This is the only assumption to double check.
In the algebraic inversion we assume no atmosphere-surface coupling, equation 11 term (1 - s * bfrfl) -> 1.
Otherwise, we only have access to rho_init, which we could use as well.
| rho_dir_dif_hi = ( | ||
| self.upsample(self.surface.wl, geom.bg_rfl) | ||
| if isinstance(geom.bg_rfl, np.ndarray) | ||
| else rho_dir_dir_hi |
There was a problem hiding this comment.
I think we may be able to reduce the repeated adjacency effect code in forward, but it could wait until after 815, where we introduce a config entry self.use_background_rfl which would make it cleaner to read to just check this bool each time within forward.py.
There was a problem hiding this comment.
Alternatively, you could bring in the config in this PR too and I can hook it up in 815. see line 43-44, https://github.com/brentwilder/isofit/blob/bg_rfl/isofit/configs/sections/surface_config.py#L33-L47
...
As I write this though, perhaps that actually does not decrease the amount of times because you still need to call self.upsample...
There was a problem hiding this comment.
Agreed that the type check is risky in the long run, but I think we can catch this up in the refactor. Let's leave for now.
Moving L_tot calc into get_L_coupled
Simplifying coupled run path Using geom to seed background in invert_simple functions Small cleanup
|
📊 Generated results:
URL: isofit/isofit-test-results#26 |
|
Summary of results from Apply OE scale tests: Magnitude 0.001 differences in reflectance solutions primarily driven by differences in aerosol solutions and Cos (i) issues. 
Reflectance residuals at blue-end follow aerosol residuals. 
Cosine angle issues are consistent: 
|
|
For last check today, but I'm happy with the complexity level (given pending refactor), and comfortable with this level of AOD-based discrepancy. With concurrence on team-call, for merge. |
The previous location of the
eq_11_termcorrection in theRT.calc_rdnfunction meant that theL_x_xvalues were inconsistently scaled betweencalc_rdnanddrdn_dsurface.This PR moves the
eq_11_termaccounting into theget_L_coupledfunction (consistent with the hays model).This does have some visible impact. Testing on a snow covered shady pixel (bright, but with high proportion of L_dif) Images courtesy of @brentwilder.
In reflectance space:
In derivative space:
Other note to keep track of that was discussed:
Introducing the eq_11 term into the forward model is another step towards more atmosphere-surface coupling within the model. This may be more accurate physics, but it potentially makes the code organization a bit more confusing. The radiative_transfer module is increasingly in charge of forward model physics that incorporate surface-atmosphere coupling despite the name implying that it is principally responsible for the radiative transfer component. The fuzziness continues with #915, a necessary change, but would further muddy the distinction and purpose of a separate
radiative_transfer_enginemodule.