Skip to content

update initial guess to be more formal and general#13

Merged
pgbrodrick merged 5 commits into
unitsfrom
units_init_guess
Jun 21, 2025
Merged

update initial guess to be more formal and general#13
pgbrodrick merged 5 commits into
unitsfrom
units_init_guess

Conversation

@pgbrodrick

Copy link
Copy Markdown
Owner

Proof it works:

Screenshot 2025-06-20 at 2 21 50 PM

@evan-greenbrg evan-greenbrg left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great. Just two small comments. Using RT.calc_RT_quantities is much cleaner.

# TODO - make this a function, and use it here and in radiatve transfer
# transmit thermal emission through the atmosphere
transup = rhi["transm_up_dir"] + rhi["transm_up_dif"]
L_up = Ls * transup

@evan-greenbrg evan-greenbrg Jun 20, 2025

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should make this TODO explicit that lines 206-207 are only compatible with rt_mode = 'transm'. If rhi["transm_up_dir"] and rhi["transm_up_dif"] are in rdn units then Ls * transup becomes a rdn * rdn multiplication.

I think we may have flagged this earlier during a working session, and also may be what the TODO is referencing.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oooh, good catch, I'll patch.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually as I look more, this should be safe with the convention we discussed this morning (which does still need to get codiied), whereby 6c components should only hold transmittances, and dir-dif (etc.) terms should hold radiances.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, I've now tried to codify this as a rule.

Comment thread isofit/inversion/inverse_simple.py
Comment thread isofit/inversion/inverse_simple.py Outdated
Comment thread isofit/inversion/inverse_simple.py Outdated
Comment thread isofit/inversion/inverse_simple.py Outdated
@unbohn

unbohn commented Jun 20, 2025

Copy link
Copy Markdown

@pgbrodrick This looks clean to me. Only extremely minor comments (see above).

So, from what I can get from this PR, it sounds like the following decisions have been made at the meeting this morning:

  • Our LUTs should always hold downwelling/upwelling/direct/diffuse terms in unitless transmittance
  • Solar irradiance, cosine of the solar zenith, and pi are not baked in into these quantities (which makes sense when having transmittance terms), but then used within ISOFIT to convert transmittance to radiance throughout the different use cases

Is that correct?

@pgbrodrick

Copy link
Copy Markdown
Owner Author

I think we're leaning towards the convention that if you're doing a 6c model, it's only the dir-dif / dif-dir terms that should get populated. IE, that holding radiance-based transmittances in that case aren't used, and that we shouldn't rely on them. Outside of the transup case, this shouldn't come up (I don't think?) from this PR....but needs to be resolved & codified more generally.

For the second bullet, I'm making the assumption (which I think is true) that calc_RT_quantities returns the directional version of those quantities. I agree we should also quantify our convention about whether sza & cosi are baked into those terms (right now I believe they are not), but that is the property of calc_RT_quantities.

@unbohn

unbohn commented Jun 20, 2025

Copy link
Copy Markdown

Got it! These are useful bullet points for future updates but shouldn't hold up this PR.

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