Absorption with cubepy #133
Conversation
…sorption_with_cubepy
…sorption_with_cubepy
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
|
@cosimoNigro @jsitarek could you look on my PR? :) |
|
I will @pawel21, sorry for the delay. I had little time in general and an issue to fix before this PR. |
| "import matplotlib.pyplot as plt\n", | ||
| "import numpy as np\n", | ||
| "from agnpy.absorption import Absorption\n", | ||
| "from agnpy.targets import SphericalShellBLR, lines_dictionary" |
There was a problem hiding this comment.
you should add some explanations what this notebook is doing
| "source": [ | ||
| "SUM_RATIO_LINES_BLR = 30.652\n", | ||
| "\n", | ||
| "def get_absor_blrs(E, L_disk, R_line, r_blob_bh, z):\n", |
There was a problem hiding this comment.
this function would be useful to be included in the main code of agnpy
XI_LINE and eps_abs should be then among the function parameters
|
sorry for a very late feedback on this |
|
digging through my old e-mails I realized that this PR is still pending. |
|
Sorry for letting it slip, will do the review later today. |
There was a problem hiding this comment.
Thanks a lot for your work @pawel21, and apologies again if I got disconnected and took a while to review it.
I think once you fix mine and @jsitarek's comments we can merge it in.
Discussing how to handle the general approach towards integration (e.g. allowing to choose between trapezoidal and adaptive integration) is something we should discuss together at a certain point in the future, along with the re-organisation of the package, not in this PR.
| # conversions | ||
| epsilon_1 = nu_to_epsilon_prime(nu, z) | ||
|
|
||
| def f(x): |
There was a problem hiding this comment.
I think it's not a good idea to define a function within another function.
I think the nested function will be recompiled each time the outer function is called. Could you move it outside?
agnpy/absorption/absorption.py
Outdated
| epsilon_line, | ||
| R_line, | ||
| r, | ||
| mu=mu_to_integrate, |
There was a problem hiding this comment.
the mu and phi arguments are not used in the function, you use directly the default mu_to_integrate and phi_to_integrate
| """ | ||
|
|
||
| tau_blr_list = [] | ||
| for freq in nu: |
There was a problem hiding this comment.
I think the problem of using integrators as cubepy, that rely on functions, instead of trapz is that you do not get directly the output as an array but you are forced to loop over each frequency value. Anyhow, I think this is good for the moment.
| blr_shells = dict() | ||
| absorption_shells = dict() | ||
|
|
||
| for line_key in lines_dictionary.keys(): |
There was a problem hiding this comment.
I am a bit lost with the calculations in this for loop.
Are you assuming that the target is a SphericalShellBLR emitting the Hbeta line? If this is not the case then I have troubles in understanding why you scale radius and luminosity to the current (arbitrary) line.
There was a problem hiding this comment.
If, on the other hand, the user has to use the Hbeta line as a target, then this should be stated explicitly somewhere.
| @@ -0,0 +1,275 @@ | |||
| { | |||
There was a problem hiding this comment.
Line #1. SUM_RATIO_LINES_BLR = 30.652
If this notebook has to be included in the docs I think it needs to be considerably improved, at the moment it has no commentary and it does not explain what its purpose is.
Reply via ReviewNB
|
Another thing that is missing is an |
Hi,
I have created PR for my modification of code for absorption using cubepy.
I added example notebook in agnpy/docs/tutorials