Refactor of forward.py.#948
Conversation
There was a problem hiding this comment.
Comments are inline! In general looks good, just catching various calls / indexing pieces that don't align w/ the refactor - I'm sure I missed some that we'll catch once we can run.
One or two of the bug catches predate the refactor, just got caught with a careful read.
I've PR'd changes based on the above into unbohn#15 - if you concur with the comments, accept and then we can merge this in.
| @@ -313,7 +302,6 @@ def invert_analytical( | |||
| # Use cached scaling factor from inital normalized inverse (outside of loop). | |||
There was a problem hiding this comment.
Could we take this chance I wonder to put Sa back inside of the inner loop here for AOE? As Phil mentioned before, even though we cache normalized inversion, technically it can change per iteration. It just doesn't right now because i=1. Please correct me if I'm wrong.
There was a problem hiding this comment.
Agreed. @pgbrodrick @evan-greenbrg Any thoughts?
| ) | ||
| if not self.atmosphere.multipart_transmittance: | ||
| r = self.atmosphere.get(x_atmosphere, geom) | ||
| if self.atmosphere.atmosphere_mode == "rdn": |
There was a problem hiding this comment.
TODO:
if possible, we should be handling this unit_mode conversion in atmosphere.py. I wonder if we could rearrange this slightly to abstract more into the atmosphere class?
Also petition to rename atmosphere_mode to something like unit_mode or the more verbose atmosphere_unit_mode
There was a problem hiding this comment.
Seems like we allow two modes: “transmittance” and “rdn”. We just need one if-statement in atmosphere.py to convert the whole LUT/interpolator object to be in rdn to have everything downstream already handled?
There was a problem hiding this comment.
That would be the most elegant solution. We just need to make sure that we don't lose accuracy when changing the order of resampling/converting between units.
| L_dif_dif = 0 | ||
|
|
||
| return ( | ||
| r, |
There was a problem hiding this comment.
We should return explicit arrays rather than the r object.
| L_coupled = [] | ||
|
|
||
| for key in self.atmosphere.coupling_terms: | ||
| L_coupled.append( |
There was a problem hiding this comment.
TODO:
find a way to abstract the unit conversions
| L_dif_dir, | ||
| L_dir_dif, | ||
| L_dif_dif, | ||
| r, |
There was a problem hiding this comment.
Could be s_alb directly
| # These are moved from radiative_transfer.py and need all references checked | ||
|
|
||
| def calc_rdn( | ||
| def drdn_datmosphere( |
There was a problem hiding this comment.
Is it worth abstracting numerical derivatives for any statevector element now that it's in forward?
|
|
||
| def drdn_dRTb( | ||
| self, x_RT, geom, rho_dir_dir, rho_dif_dir, rho_dir_dif, rho_dif_dif, Ls, rdn | ||
| def drdn_datmosphereb( |
There was a problem hiding this comment.
Similarly, is it worth abstracting to any vector quantity if this now lives in forward?
Refactor forward model inverse
Initial draft of the
forward.pyrefactoring. Same as #947, this is work in progress and more updates are to come.Additional thoughts that came up during editing the code:
drdn_dsurfaceandanalytical_modeltoforward.pyas well. Currently, those live in the surface object, but take both surface and atmosphere RT quantities as input. Might be challenging though due to the class inherence between our different surface modules.invert_algebraic()now requires theForwardModelobject as input to have access to functions likecalc_atmosphere_quantities(). This should be fine, but we need to carefully check if this causes issues down the line.