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

Bound-free interactions #325

Merged
merged 16 commits into from
Jul 8, 2015
Merged

Bound-free interactions #325

merged 16 commits into from
Jul 8, 2015

Conversation

chvogl
Copy link
Contributor

@chvogl chvogl commented Jun 11, 2015

Start of continuum implementation:
for testing purposes bound-free interactions are currently treated as true absorptions and are described
by a fixed grey opacity

TODOs

  • added bound-free absorption
  • add a flag to switch this on or off
  • unit tests
  • offline quantitative test

@wkerzendorf
Copy link
Member

Great that looks like a good start. I need to have a read through it.

// Compute the continuum oddities for a real packet
// calculate_chi_bf(packet, storage);

chi_boundfree = 0.2e-15; //calculate_chi_bf(packet, storage);
Copy link
Member

Choose a reason for hiding this comment

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

I think for these values it would be useful to write getters. these can for now produce preset results, but later be linked to atomic data.

chvogl added 3 commits June 16, 2015 10:35
 -implemented function to calculate chi_bf:
   use rudimentary hardcoded list of continuum
   edges (similar to line_list) to test frequency dependent  absorption
- introduced a dummy list of frequencies of continuum edges
   similar to the list of line transitions
- added function to calculate bound-free opacities
   in a hydrogenic approximation using dummy data
@wkerzendorf
Copy link
Member

hi @chvogl can you have a look at the bullet points in the PR. You probably have already added a flag to switch this on/off (then just tick this). With the unit tests @kaushik94 can help for this. Thanks

@chvogl
Copy link
Contributor Author

chvogl commented Jun 24, 2015

hi @wkerzendorf. I have a flag to switch off the continuum processes. However, as it is implemented now it is only a temporary solution. Are the offline quantitative tests something I should concern myself with right now?

@wkerzendorf
Copy link
Member

@chvogl great! A current temporary solution is great for this PR. The question about the quantitative tests is: It depends on what you are working on right now. You can also add more checkmarks on the PR list tell us what you think still needs to be done for this first part of the BF addition. have a look at #332 how @aoifeboyle does that. Finally, you can also always put a PNG of a plot in the description of the PR (you can always change it as you add more commits). let me know if there's anything I can help you with.

@ssim
Copy link
Contributor

ssim commented Jun 26, 2015

@chvogl I think the idea for a quantitative test is that you make some simple test problem up (a constant density spherical slab or whatever) where you can compute the expected depth of a continuum edge - and then (offline) check that the code produces it. Eventually this would become a proper test in the code, but for now having such a check (with appropriate yaml file to run it) would be good. @unoebauer will be able to help propose/guide on this too, I think.

chvogl added 2 commits June 29, 2015 16:41
-implemented a function which returns the frequency dependent
  bf cross-section (based on dummy data)
-selection of i-packet or k-packet creation in a
  bf-absorption
@chvogl
Copy link
Contributor Author

chvogl commented Jul 3, 2015

Quantitative test:
two Tardis simulations with and without bound-free interactions (see plot below in green and blue)
Parameters for simulations:

  • disable_electron_scattering: yes
  • velocity: start : 1 km/s ; stop : 20000 km/s
  • time_explosion: 1300 day
  • density type : uniform ; value : 1e-42 g/cm^3
  • all other parameters are as in the tardis_example.yml

Parameters for bound-free absorption:

  • frequency of continuum absorption edges : 9.0e14, 8.223e14, 6.0e14, 3.5e14, 3.0e14 Hz
  • chi_bf_partial = 0.0075* 0.25e-15 1/cm;
    opacities = [chi_bf_partial, 0.0, 2.0 * chi_bf_partial, 0.3 * chi_bf_partial, 2.0 * chi_bf_partial]

Analytic solution:

  • one approximation: due to the large ratio between the radius of the inner and outer boundary
    (1/20000) all rays are essentially moving radially
  • O(v/c) accuracy in transforming between observer and comoving frame
  • we can obtain the ratio between the luminosity with and without bound-free absorption from the
    analytic solution of the radiative transfer equation (see plot below in red)
    bf_test

@unoebauer
Copy link
Contributor

Hi @chvogl @wkerzendorf @ssim,

I think this test of the bf processes looks really good and encouraging. If you agree, I would call this milestone (offline testing) closed. Since only the unit tests are still missing, we could already merge now. What do you think?

@wkerzendorf
Copy link
Member

Sounds good to me. The last thing, that I would want to do is to link the script for the analytical model and plots, so that we can later possibly convert this into a test. I suggest using github gist (it will take you 30 seconds to put it there) and then put the link here.

@chvogl
Copy link
Contributor Author

chvogl commented Jul 3, 2015

The program with the analytical solution can be found here: https://gist.github.com/chvogl/f193ce79d8ed19274ed8#file-quantitative_test-py

@wkerzendorf
Copy link
Member

@chvogl thank you very much! @ssim I'll let you merge. Looks good from my side.

@unoebauer
Copy link
Contributor

@wkerzendorf @ssim - please wait with the merging. @chvogl is currently refining the switch implementation so that it also affects the bf data preparation processes.

@ssim
Copy link
Contributor

ssim commented Jul 3, 2015

Ok - let me know when you are ready! I'm also very pleased with this and happy to merge whenever @chvogl has had time to deal with the switch (as above)

….c to montecarlo.pyx

-disabled passing of continuum dummy data if continuum processes are switched off
@chvogl
Copy link
Contributor Author

chvogl commented Jul 3, 2015

@ssim I still want to do some cleanup. I will get back to you as soon as I'm finished.

@wkerzendorf
Copy link
Member

@chvogl the merge of the parallel stuff screwed things up slightly. you will need to manually merge. Let me know if you have any questions about it.

@chvogl
Copy link
Contributor Author

chvogl commented Jul 7, 2015

@ssim @wkerzendorf I think we can merge now.

@ssim
Copy link
Contributor

ssim commented Jul 7, 2015

ok - I will merge today, unless @wkerzendorf has any objections?

@ssim
Copy link
Contributor

ssim commented Jul 8, 2015

I'm happy to merge this as it, but one thing to consider @chvogl @wkerzendorf @unoebauer ....is this all thread safe for the the openmp approach being used now?

E.g. I suspect that chi_bf_tmp_partial at least needs to be thread private, although there may be other things? Is this already taken care of @chvogl ?

@ssim
Copy link
Contributor

ssim commented Jul 8, 2015

By the way, even if it's not thread safe yet, I'd be inclined to merge anyway and just make an issue that we need to come back at some point and check this is all compatible with openmp (I don't think we necessarily need to hold up progress towards accepting this PR request) - thoughts?

@chvogl
Copy link
Contributor Author

chvogl commented Jul 8, 2015

@ssim I'm aware that the chi_bf_tmp_partial is not thread safe yet. When I asked @unoebauer and @wkerzendorf about it, the general opinion was that this is something, which can be taken care of later on. Thus I would agree with you that we merge now and open an issue about the openmp compatibility.

@unoebauer
Copy link
Contributor

Hi @ssim, @wkerzendorf:

As @chvogl said, we discussed yesterday potential incompatibilities of the bf scheme with the parallel version of Tardis. Since the current bf implementation is still at the early development stage and likely to change, my opinion was to not worry about this for the first merge. But we should definitely discuss this in more detail at the WW. Bottom line: I'd be in favour of merging the current version and opening and issue.

@wkerzendorf
Copy link
Member

@chvogl @unoebauer @ssim looks good ... am merging. Please open an issue @chvogl to remind us that we need to make the mentioned array. Great job!

wkerzendorf added a commit that referenced this pull request Jul 8, 2015
@wkerzendorf wkerzendorf merged commit f0bbeb7 into tardis-sn:master Jul 8, 2015
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.

4 participants