Simplify to single RTE per run#915
Conversation
|
📊 Generated results:
URL: isofit/isofit-test-results#12 |
|
|
||
| # Make sure the length of the config statevectores match the engine's assumed statevectors | ||
| if (expected := len(config.statevector.get_element_names())) != ( | ||
| got := len(rte.indices.x_RT) | ||
| ): | ||
| error = f"Mismatch between the number of elements for the config statevector and LUT.indices.x_RT: {expected=}, {got=}" | ||
| Logger.error(error) | ||
| raise AttributeError(error) | ||
|
|
There was a problem hiding this comment.
I wonder if this error should actually remain. It was my thinking that originally this targeted at VSWIR v TIR, but could happen in the VSWIR only case. e.g., someone has co2, h2o, and aod in the lut and the state vector only has two of these?
There was a problem hiding this comment.
Agreed, I think the only minor change I would suggests is to change the check from between:
config.statevector.get_element_names() and self.engine.indices.x_RT
to between self.statevec_names and ``self.engine.indices.x_RT`
Purely semantic. The value of the check is to make sure you're not passing a pre-built lut with a mismatching lut grid from the config. Are there other cases that would hit this?
I think this is a reasonable place for this check.
There was a problem hiding this comment.
Woops, I misread the diffs, it just got shifted up a few lines, not removed. But I will go ahead and switch to your suggested check here.
evan-greenbrg
left a comment
There was a problem hiding this comment.
Great cut @brentwilder
I agree that merging this before the RT refactor would help simplify the two-layered classes and make our lives easier for the task.
My comments are minor, and could be pushed to different PRs or the refactor.
|
|
||
| # Make sure the length of the config statevectores match the engine's assumed statevectors | ||
| if (expected := len(config.statevector.get_element_names())) != ( | ||
| got := len(rte.indices.x_RT) | ||
| ): | ||
| error = f"Mismatch between the number of elements for the config statevector and LUT.indices.x_RT: {expected=}, {got=}" | ||
| Logger.error(error) | ||
| raise AttributeError(error) | ||
|
|
There was a problem hiding this comment.
Agreed, I think the only minor change I would suggests is to change the check from between:
config.statevector.get_element_names() and self.engine.indices.x_RT
to between self.statevec_names and ``self.engine.indices.x_RT`
Purely semantic. The value of the check is to make sure you're not passing a pre-built lut with a mismatching lut grid from the config. Are there other cases that would hit this?
I think this is a reasonable place for this check.
|
|
||
| # 1c or 3c? | ||
| self.multipart_transmittance = self.rt_engines[0].multipart_transmittance | ||
| self.multipart_transmittance = self.engine.multipart_transmittance |
There was a problem hiding this comment.
Very minor, and is part of the motivation for the refactor.
Do both self.engine and self require multipart_transmittance?
There was a problem hiding this comment.
Only used in radiative_transfer.py, and agreed already stashed in self via self.engine .. and so this can be reduced for sure.
| "configs/run/joint_isofit_with_prof_WATER_nogrid.json", level="DEBUG" | ||
| ) | ||
| model.run() | ||
| # @pytest.mark.xfail |
There was a problem hiding this comment.
Probably a TODO item to be cleaned up later.
We should decide on whether we want to get the IR test running. We should add a new pytest marker s.t. the test isn't executed.
There was a problem hiding this comment.
Yeah I would defer to you what you think. So do you mean to say to leave uncommented, and just switching out @pytest.mark.examples for something like @pytest.mark.skip(reason="IR implementation needs to be updated")?
Redefine RT Engine config to always be dict (no longer list) Initialize new rt engine setup Initialize new rt engine setup Comment out ThermalIR test that will need to be reworked in the future Correct to single engine in multipart check Correct typo to config order which is forward_model, radiative_transfer, then engine Mark skip for IR pytest and reduce redundancy in radiative_transfer.py
366ccc1 to
3e94e6c
Compare
The proposed change to config:
current:
["radiative_transfer"]["radiative_transfer_engines"]["vswir"]["engine_name"] = "sRTMnet"proposed:
["radiative_transfer"]["engine"]["engine_name"] = "sRTMnet"And so, this works just fine however, the test data will need to be updated before (isofit/isofit-tutorials#45) we are able to merge this PR.