Added check in calc_rdn for s_alb = 0#645
Merged
Merged
Conversation
pgbrodrick
reviewed
Mar 10, 2025
…e 1c model is used.
unbohn
approved these changes
Mar 10, 2025
Collaborator
|
@pgbrodrick I pushed a solution that handles both the 1c and 4c cases in one radiance equation, and at the same time, prevents zeros in the spherical albedo term to cause any issues down the line. Feel free to merge if you concur. |
Collaborator
|
Nice algebraic trick @unbohn - this is also generally more numerically stable, so nothing but a win. I'll wait for the checks to complete for formality, and then merge. Thanks! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Cases when the spherical albedo term in the lut table are 0 currently produce nan values, which leads exceptions when calculating inverse matrices.
When
s_albcontains nans,calc_rdnreturns a vector with partial nans (indexes wheres_alb = 0). The resample function does not include a check for nans within thexvector (onlyH). Whenxcontains partial nans,resamplereturns a vector of full nans.As far as I can tell, this is only an issue with the 1c model. This fix adds a narrow check to the
calc_rdnfunction within the 1c if statement for cases whens_alb = 0. I tried to make it verbose with a debug log, which could be warning if we want it to always be printed.With how things are currently structured, the value we replace the nans with does not matter, as the 1c model will always return 0s where
s_alb = 0. Potentially a behavior this doesn't share with Isofit 2.