Absorption with cubepy #133

pawel21 · 2023-01-10T18:13:29Z

Hi,

I have created PR for my modification of code for absorption using cubepy.
I added example notebook in agnpy/docs/tutorials

…sorption_with_cubepy

review-notebook-app · 2023-01-10T18:13:33Z

Check out this pull request on

See visual diffs & provide feedback on Jupyter Notebooks.

Powered by ReviewNB

pawel21 · 2023-03-12T02:38:08Z

@cosimoNigro @jsitarek could you look on my PR? :)

cosimoNigro · 2023-03-19T18:50:13Z

I will @pawel21, sorry for the delay. I had little time in general and an issue to fix before this PR.
Will try in the next two weeks.

agnpy/absorption/absorption.py

docs/tutorials/absorption_in_BLR_with_cubepy.ipynb

jsitarek · 2023-03-31T15:35:06Z

docs/tutorials/absorption_in_BLR_with_cubepy.ipynb

+   "source": [
+    "SUM_RATIO_LINES_BLR = 30.652\n",
+    "\n",
+    "def get_absor_blrs(E, L_disk, R_line, r_blob_bh, z):\n",


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

jsitarek · 2023-03-31T15:40:18Z

sorry for a very late feedback on this
@pawel21 - I left a few comments on the PR
@cosimoNigro - how do you prefer to proceed - should this function substitute the current integration? or should it be added as a separate function (like Paweł did now)? In the latter case a test comparing the two methods should be added.
The former case might be more reasonable, because this method is more precise and do not have those crazy memore requirements of the standard integration scheme.

…sorption_with_cubepy

jsitarek · 2023-07-06T09:15:24Z

digging through my old e-mails I realized that this PR is still pending.
@pawel21 I think that still 2 of my comments are not implemented/responded
@cosimoNigro any feedback about the general implementation and relation to the older code?

cosimoNigro · 2023-07-10T08:05:22Z

Sorry for letting it slip, will do the review later today.

cosimoNigro

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.

cosimoNigro · 2023-07-16T15:19:08Z

agnpy/absorption/absorption.py

+        # conversions
+        epsilon_1 = nu_to_epsilon_prime(nu, z)
+
+        def f(x):


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?

cosimoNigro · 2023-07-16T16:11:01Z

agnpy/absorption/absorption.py

+        epsilon_line,
+        R_line,
+        r,
+        mu=mu_to_integrate,


the mu and phi arguments are not used in the function, you use directly the default mu_to_integrate and phi_to_integrate

cosimoNigro · 2023-07-16T16:17:07Z

agnpy/absorption/absorption.py

+        """
+
+        tau_blr_list = []
+        for freq in nu:


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.

true, but since you do not have arrays you have much smaller memory usage, and the integration module automatically selects the points to evaluate the function in an optimal way, so a simple loop over frequencies is a small price to pay :-)

agnpy/absorption/absorption.py

cosimoNigro · 2023-07-16T16:33:18Z

docs/tutorials/absorption_in_BLR_with_cubepy.ipynb

@@ -0,0 +1,275 @@
+{


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

Let's remove the notebook for now so we can go further. I would also like to develop other features, but it will be easier if the code for calculation absorption with cubepy is in the main repo. In the next step, I will add notebooks with explanations for the multi-shell BLR model.

cosimoNigro · 2023-07-16T16:34:18Z

Another thing that is missing is an absorption function returning the actual attenuation factor in the cubepy case, now only tau is computed.

…sorption_with_cubepy

pgliwny · 2024-10-02T12:59:38Z

Another thing that is missing is an absorption function returning the actual attenuation factor in the cubepy case, now only tau is computed.

What do you mean? Which function is missing? I tried to add a function similar to a function for another target.

pgliwny · 2024-10-02T13:54:50Z

@jsitarek @cosimoNigro Could you please review my code?
I have addressed all of your suggestions and modified the code accordingly. I will be using this code to model absorption for new source in near future and to add a fit that allows for the consideration of multiple shells included in the BLR. For this purpose, it would be easier to have this code in the main repository for further improvements.

jsitarek · 2024-10-03T01:00:30Z

Hi @pgliwny
The automatic CI checks fail:

FAILED agnpy/fit/tests/test_fit.py::TestFit::test_mrk_421_fit - IndexError: boolean index did not match indexed array along dimension 2; dimension is 1 but corresponding boolean dimension is 9
FAILED agnpy/fit/tests/test_fit.py::TestFit::test_pks_1510_fit - ValueError: non-broadcastable output operand with shape (6,1,1) doesn't match the broadcast shape (6,1,6)

maybe your PR changed something in the API

EDIT: I had a deeper look and the new code does not modify any of the existing functions so it should not be the reason for the fail of the tests.

jsitarek · 2024-10-03T01:02:39Z

agnpy/absorption/absorption.py

+        Returns
+        -------
+        float
+            The value of the integrand for the given parameters.


it would be useful to specify in which units the integrand is specified (I think it should be cm)

agnpy/absorption/absorption.py

jsitarek · 2024-10-03T01:26:50Z

agnpy/absorption/absorption.py

+            **note** these are observed frequencies (observer frame)
+        z : float
+            redshift of the source
+        mu_s : float


I think this function is following evaluate_tau_blr that is only valid for mu_s=1 (otherwise integration over path of the photon and over the distance from the black hole is not equivalent), so it should not have it as a parameter (@cosimoNigro can you also have a look at this?)

jsitarek · 2024-10-03T01:34:42Z

agnpy/absorption/absorption.py

+        eps_abs: float, optional
+            Absolute precision for the calculation, default is 1e-6
+        """
+        SUM_RATIO_LINES_BLR = 30.652


hardcoding this is not safe, you should sum up the lines from the dictionary that you will use

jsitarek · 2024-10-03T01:38:29Z

agnpy/absorption/absorption.py

+        XI_TARGET_LINE = self.target.xi_line
+        tau_z_all = np.zeros(nu.shape)
+
+        blr_shells = dict()


why do you define those dictionaries? I guess it is a remnant of a script where you were also plotting the effect of individual lines, but this is either way not possible this function.

here you are using the dictionaries only to sum up over all the lines, so you could instead created SphericalShellBLR and Absorption as temporary variables directly in the for loop

jsitarek · 2024-10-03T01:46:23Z

agnpy/absorption/absorption.py

+        and applies the computation to all the available BLR lines (shells) using
+        their respective ratios.
+
+        Parameters


how about adding a dictionary 'lines' to the parameters (and setting it by default to lines_dictionary)
this would allow to run the function with limited number of lines (selecting only the most important ones for speed) or with modified dictionary (if we have more accurate measurements for a particular source). It would also facilitate tests because you could then implement a quick test on a dictionary of 2 lines

Great idea, but what do you think about doing it in the "second" iteration (after merging this code with all lines into the main branch)?

if you prefer ok, but note that this is really minor change to the code, just adding a new parameter to the function, with a default value and then in the line 637 using this parameter instead of lines_dictionary.
And it would simplify implementation of my comment about tests

jsitarek · 2024-10-03T01:47:23Z

agnpy/absorption/tests/test_absorption.py

@@ -228,6 +228,31 @@ def test_abs_dt_vs_point_source(self):
        # requires a 10% deviation from the two SED points
        assert check_deviation(nu, tau_dt, tau_ps_dt, 0.1)

+    def test_abs_blr_cubepy(self):


the test as it is written now does not test much: only if you do not have a crash because of not matching API, or some internal inconsistency in the code.
Instead you could run a test of comparing tau_blr_cubepy output with tau_blr

jsitarek

I left some comments. In particular the test should be rewritten.

pawel21 added 8 commits November 26, 2021 15:56

Merge branch 'pylint_and_flake8' of https://github.com/cosimoNigro/agnpy

9bfc005

Merge branch 'master' of https://github.com/cosimoNigro/agnpy

3bb4a33

calcurate absorption using cubepy

a23cab0

add set up error to integrate

0ca6f70

iterate freq to compute tau_blr

4b9f0e3

Merge branch 'master' of https://github.com/cosimoNigro/agnpy into ab…

be9979a

…sorption_with_cubepy

Merge branch 'master' of https://github.com/cosimoNigro/agnpy into ab…

eff6965

…sorption_with_cubepy

add notebook with absorption in BLR with cubepy

bcd1888

jsitarek requested changes Mar 31, 2023

View reviewed changes

pawel21 added 5 commits April 6, 2023 14:39

Merge branch 'master' of https://github.com/cosimoNigro/agnpy into ab…

ffdecbe

…sorption_with_cubepy

remove l_size

403a0bb

start develop funtion to calcurate absorption over all line for BLR

c0b6cfa

added fun to calculate absorption in all BLR lines

8e21ba8

nodified notebook to new function

57d67bc

cosimoNigro reviewed Jul 16, 2023

View reviewed changes

pawel21 added 7 commits April 26, 2024 16:07

add test for absorption in BLR with cubepy

7039b98

remove not used parameters

dd28724

Add more clear explanations for docs string about calc tau in BLR

92eb17b

Add function to compute inegrand for cubepy

e75d6d8

Merge branch 'master' of https://github.com/cosimoNigro/agnpy into ab…

d5b4778

…sorption_with_cubepy

Add environment dependencies for cubepy

a81e789

Fix environment dependencies

eeed0a9

pawel21 added 4 commits June 3, 2024 19:10

Check target name during calc tau

3aabb7c

Fix problem with cubepy instalion

e6b73df

add cubepy requires to setup.py

269eec2

work on exmpale notebook

70b6ef8

remove notebook from docs

246a7d3

jsitarek reviewed Oct 3, 2024

View reviewed changes

agnpy/absorption/absorption.py Outdated Show resolved Hide resolved

jsitarek reviewed Oct 3, 2024

View reviewed changes

agnpy/absorption/absorption.py Outdated Show resolved Hide resolved

jsitarek reviewed Oct 3, 2024

View reviewed changes

agnpy/absorption/absorption.py Outdated Show resolved Hide resolved

jsitarek reviewed Oct 3, 2024

View reviewed changes

jsitarek requested changes Oct 3, 2024

View reviewed changes

make code more concise and renamed function

ede4818

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Absorption with cubepy #133

Absorption with cubepy #133

pawel21 commented Jan 10, 2023

review-notebook-app bot commented Jan 10, 2023

pawel21 commented Mar 12, 2023

cosimoNigro commented Mar 19, 2023

jsitarek Mar 31, 2023

jsitarek commented Mar 31, 2023

jsitarek commented Jul 6, 2023

cosimoNigro commented Jul 10, 2023

cosimoNigro left a comment

cosimoNigro Jul 16, 2023

pawel21 Jun 3, 2024

cosimoNigro Jul 16, 2023

pawel21 Apr 26, 2024

cosimoNigro Jul 16, 2023

jsitarek Jun 3, 2024

cosimoNigro Jul 16, 2023 •

edited

Loading

pgliwny Oct 2, 2024

cosimoNigro commented Jul 16, 2023

pgliwny commented Oct 2, 2024

pgliwny commented Oct 2, 2024

jsitarek commented Oct 3, 2024 •

edited

Loading

jsitarek Oct 3, 2024 •

edited

Loading

jsitarek Oct 3, 2024

jsitarek Oct 3, 2024

jsitarek Oct 3, 2024 •

edited

Loading

jsitarek Oct 3, 2024

pgliwny Oct 9, 2024

jsitarek Oct 10, 2024

jsitarek Oct 3, 2024

jsitarek left a comment

Absorption with cubepy #133

Are you sure you want to change the base?

Absorption with cubepy #133

Conversation

pawel21 commented Jan 10, 2023

review-notebook-app bot commented Jan 10, 2023

pawel21 commented Mar 12, 2023

cosimoNigro commented Mar 19, 2023

Choose a reason for hiding this comment

jsitarek commented Mar 31, 2023

jsitarek commented Jul 6, 2023

cosimoNigro commented Jul 10, 2023

cosimoNigro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cosimoNigro Jul 16, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cosimoNigro commented Jul 16, 2023

pgliwny commented Oct 2, 2024

pgliwny commented Oct 2, 2024

jsitarek commented Oct 3, 2024 • edited Loading

jsitarek Oct 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsitarek Oct 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsitarek left a comment

Choose a reason for hiding this comment

cosimoNigro Jul 16, 2023 •

edited

Loading

jsitarek commented Oct 3, 2024 •

edited

Loading

jsitarek Oct 3, 2024 •

edited

Loading

jsitarek Oct 3, 2024 •

edited

Loading