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

nGray_Analytic_MultigroupOpacity to Compound_Analytic_MultigroupOpacity #619

Merged
merged 2 commits into from
May 21, 2019

Conversation

kgbudge
Copy link
Contributor

@kgbudge kgbudge commented May 13, 2019

Likewise rename nGray_Analytic_Odfmg_Opacity to Compound_Analytic_Odfmg_Opacity.

Change Compound_Analytic_MultigroupOpacity to use group structure when calculating opacities, to make it consistent with Compound_Analytic_Odfmg_Opacity previously used in Capsaicin.

Background

The Capsaicin project is, so far as we know, the only client for the OdfmgOpacity class hierarchy. This was written to support experiments on using opacity distribution functions for thermal transport calculations. The results were not encouraging and no further work has been done in this direction in years, nor do we foresee any further work in the future. Capsaicin has therefore abandoned support of opacity distribution functions.

This removes significant code bloat from Capsaicin, but falling back to MultigroupOpacity from Odfmg_Opacity revealed that the nGray_Analytic_MultigroupOpacity class does not produce the same opacities as nGray_Analytic_Odfmg_Opacity does for a single distribution band. Further investigation showed thatn nGray_Analytic_MultigroupOpacity ignores frequency dependence in its opacity calculation.

It was not clear at first whether this was deliberate. Digging into the code convinced me that I had not intended nGray_Analytic_MultigroupOpacity to have the same opacity in all groups in all cases. The name seems confusing enough that Compound_Analytic_... seemed more appropriate to capture the concept, which is of an opacity built on a set of individual group models.

Purpose of Pull Request

Give nGray ... classes a more appropriate name, Compound...
Make Compound_Analytic_MultigroupOpacity match Compound_Analytic_OdfmgOpacity for a single band per group.

Description of changes

Renamed classes.
Modified opacity calculation loop to use the frequency dependent interface of the Analytic_Opacity_Model class.

Status

…upOpacity to better reflect its purpose.

Likewise rename nGray_Analytic_Odfmg_Opacity to Compound_Analytic_Odfmg_Opacity.

Change Compound_Analytic_MultigroupOpacity to use group structure when calculating opacities, to make it consistent with Compound_Analytic_Odfmg_Opacity previously used in Capsaicin.
@codecov
Copy link

codecov bot commented May 13, 2019

Codecov Report

Merging #619 into develop will increase coverage by <.1%.
The diff coverage is 100%.

@@            Coverage Diff            @@
##           develop    #619     +/-   ##
=========================================
+ Coverage     93.7%   93.7%   +<.1%     
=========================================
  Files          380     380             
  Lines        17799   17805      +6     
=========================================
+ Hits         16688   16694      +6     
  Misses        1111    1111

@attom
Copy link
Contributor

attom commented May 20, 2019

Everything looks good except for the naming, which I don't quite understand. You're renaming "nGray" because the opacities are not G copies of a gray opacity, right? But what makes them "Compound"? They're just a MG or MG+MB opacity, where there's a single formula used to figure out the opacity in all groups?

@keadyk
Copy link
Collaborator

keadyk commented May 21, 2019

Sorry @KineticTheory, this totally slipped my mind! Reviewing now :)

@kgbudge
Copy link
Contributor Author

kgbudge commented May 21, 2019

Everything looks good except for the naming, which I don't quite understand. You're renaming "nGray" because the opacities are not G copies of a gray opacity, right? But what makes them "Compound"? They're just a MG or MG+MB opacity, where there's a single formula used to figure out the opacity in all groups?

Each group is linked to its own Analytic_Opacity_Model (though often it's the same model for all groups, via shared_ptr) and so the Compound_Analytic_MultigroupOpacity is "compounded" of Analytic_Opacity_Models.

Copy link
Collaborator

@keadyk keadyk left a comment

Choose a reason for hiding this comment

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

This looks good to me!

@keadyk keadyk merged commit e079dab into lanl:develop May 21, 2019
@kgbudge kgbudge deleted the nGray_to_Compound branch May 21, 2019 14:22
@attom
Copy link
Contributor

attom commented May 21, 2019

That makes sense. Looks good to me, then.

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.

4 participants