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

added factors of geo.fill to routines which increment heating estimators #129

Merged
merged 2 commits into from
Nov 25, 2014

Conversation

jhmatthews
Copy link
Collaborator

When microclumping was first implemented- and even when I updated the routines- we only included factors of geo.fill in the calculated opacities in the following routines:

  • in calculate_ds when we use opacities to work out when a photon scatters, both in continuum and line opacities
  • in the tau_sobolev which is passed to the routines which calculate the bb/line heating- in both macro-atom and normal mode

We therefore failed to include factors of geo.fill when incrementing some of our heating estimators for each cell. This request should be discussed before merging. I have added the following factors of geo.fill:

  • in kappa_compton, kappa_ind_comp in compton.c. These propagate correctly into the routines which calculate the heating in macro-atom (bf_estimators_increment) and non macro-atom (radiation) mode.
  • in kappa_ff and kappa_bf, as above
  • removed some factors in radiation() as the factor is now intrinsic to the relevant kappa routine
  • in the calculation of bf opacities in calculate_ds and radiation

This may have a significant impact on the clumped models I've run, so take those results with a pinch of salt.

There's an outstanding problem here which is that I believe some volumes are not done consistently, i.e. they use the actual volume of the cell when they should use the filled volume. This is true of both diagnostic quantities like the IP, but also normalisations which we do in macro-atoms. I'm not sure these instances will affect actual resulting calculations but they should be done self-consistently. I'm trying to review any instance where we calculate things based on densities and volumes.

@kslong
Copy link
Collaborator

kslong commented Nov 24, 2014

I believe there are two volumes involved in our structures. The one in the wind pointer and the one in the plasma pointer. The one that was modified for clumping was in the plasma pointer, and this is the volume that anything having to do with emissivity should point to. If this is done then I think we should be OK from the emissivity perspective.

James, I’m surprised by kappa_ff, etc. I thought we changed the density correctly and therefore this should come out in the wash, since we allowed for clumping in calculating tau.

@jhmatthews
Copy link
Collaborator Author

Right. But unfortunately:

  • we didn't modify most of the emissivities to use the plasma volume - they still use wind volume in many cases and
  • we did allow for clumping in calculating tau, but then we pass a ds, not a tau, to the estimator routines (e.g. density) and they recalculated tau based on a kappa which was not correctly multiplied by geo.fill.

Of course, this unfortunately means that all that lovely line emission in my clumped QSO model is probably complete garbage. Hopefully the ionization state is close to correct...

@jhmatthews
Copy link
Collaborator Author

In my latest commit, I've corrected what I believe to be the quantities which should use the 'filled volume' rather than the 'cell volume'. These include emissivities, diagnostics such as J or IP, cooling normalisations etc, and in some cases the factors would cancel with previous factors. There are a lot of cases where we missed this, which was pretty stupid.

Would be grateful if someone e.g. @kslong could check what I've done and verify that they agree we ar multiplying the correct quantities.

jhmatthews added a commit that referenced this pull request Nov 25, 2014
added factors of geo.fill to routines which increment heating estimators
@jhmatthews jhmatthews merged commit 026fe26 into sirocco-rt:dev Nov 25, 2014
@jhmatthews jhmatthews deleted the clump_estimators branch June 3, 2016 15:40
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.

2 participants