Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use std::expm1() in conversion from Planck to Rosseland. #173

Merged
merged 3 commits into from
Mar 6, 2017

Conversation

dholladay00
Copy link
Contributor

@RyanWollaeger
Copy link
Contributor

Several machines appear to have caught some failed assertions associated with this header:
https://rtt.lanl.gov/cdash/testDetails.php?test=489229&build=856

Copy link
Contributor

@RyanWollaeger RyanWollaeger left a comment

Choose a reason for hiding this comment

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

Some tests are failing.

@dholladay00
Copy link
Contributor Author

Yep, I built Draco and am seeing that Assertion: rosseland >= 0.0, failed in /users/dholladay00/repos/Draco/src/cdi/CDI.hh, line 747.

So, it's definitely something wrong with my change.

@KineticTheory
Copy link
Collaborator

The old if/else logic was in place to avoid division by zero when nu is very close to zero. That might need to go back in.

@RyanWollaeger RyanWollaeger merged commit 168c3f2 into lanl:develop Mar 6, 2017
@dholladay00
Copy link
Contributor Author

dholladay00 commented Mar 7, 2017

More extensive tests failed due to overflow. This is probably occurring when freq is large, thus making expm1(freq) overflow (that is unverified but my guess without further investigation). This was the wisdom baked in to the previous way of multiplying numerator and denominator by exp(-freq). Even in this form, there is a way to use expm1 in that 1 - exp(-freq) = -expm1(-freq).

thus it would be exp_freq * freq * freq_3 / -std::expm1(-freq)

Of course looking at the order of operations could be useful as well to try and group operations such that each result remains as close to O(1) as possible.

Rolling back all of the changes is an option that we know won't hurt anything, but I'd like to see if the new small frequency approximation breaks anything b/c it should be more accurate (I also highly doubt it's the culprit in this case).

@keadyk keadyk mentioned this pull request Mar 7, 2017
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants