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

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

agnpy/absorption/absorption.py

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

Done, added test_tau_accuracy_standard_vs_cubepy

jsitarek

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

jsitarek · 2024-12-23T12:59:59Z

agnpy/absorption/absorption.py

+
+    def tau_blr_lines_cubepy(self, nu, lines_list, eps_abs=1e-6):
+        """
+        Calculate the absorption in all available BLR (Broad Line Region) shells.


"all available BLR shells" is a copy paste from the original function, better write "multiple BLR ..." or something like this since now you specify exaclty the list of lines to consider

jsitarek · 2024-12-23T13:04:34Z

agnpy/absorption/absorption.py

+             Optical depth values corresponding to the input frequencies.
+        """
+
+        sum_ratio_lines = sum(lines_dictionary[line]['L_Hbeta_ratio'] for line in lines_list)


you need to introduce a check or even better throw an exception if any of the requested line is not in lines_dictionary.keys()

jsitarek · 2024-12-23T13:06:42Z

agnpy/absorption/absorption.py

+        ----------
+        nu : numpy.ndarray
+            Frequency array for which the absorption will be calculated.
+        lines_list : list of str


if you set this as default to lines_dictionary.keys() then you do not need tau_blr_all_lines_cubepy function any more

jsitarek · 2024-12-23T13:08:14Z

agnpy/absorption/absorption.py

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


copying the comment from the previous function:

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

codecov · 2025-01-21T15:14:24Z

Codecov Report

Attention: Patch coverage is 67.01031% with 32 lines in your changes missing coverage. Please review.

Project coverage is 96.31%. Comparing base (506c038) to head (fe80a75).

Files with missing lines	Patch %	Lines
agnpy/absorption/absorption.py	48.38%	32 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #133      +/-   ##
==========================================
- Coverage   97.13%   96.31%   -0.83%     
==========================================
  Files          42       42              
  Lines        3459     3554      +95     
==========================================
+ Hits         3360     3423      +63     
- Misses         99      131      +32

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

…gnpy into absorption_with_cubepy

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 3 commits June 7, 2024 15:26

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

agnpy/absorption/absorption.py Show resolved Hide resolved

jsitarek reviewed Oct 3, 2024

View reviewed changes

jsitarek requested changes Oct 3, 2024

View reviewed changes

pawel21 added 2 commits October 9, 2024 16:58

make code more concise and renamed function

ede4818

add unit in doc and add tau_blr_lines_cubepy_function

6c00ba1

jsitarek reviewed Dec 23, 2024

View reviewed changes

pawel21 added 2 commits January 21, 2025 15:56

Merge branch 'master' into absorption_with_cubepy

ea38e31

improved docs description

af96f66

pawel21 added 4 commits January 22, 2025 17:25

start write test for compare tau blr standard with cubepy

33f96df

test

1ca2ef9

Merge branch 'absorption_with_cubepy' of https://github.com/pawel21/a…

9cd77ea

…gnpy into absorption_with_cubepy

add test for tau calcuration with standard and cubepy method

fe80a75

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 •

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

pgliwny Dec 18, 2024

jsitarek Oct 3, 2024

jsitarek Oct 3, 2024

jsitarek Oct 3, 2024 •

edited

Loading

jsitarek Oct 3, 2024

pawel21 Jan 23, 2025

jsitarek left a comment

jsitarek Dec 23, 2024

pawel21 Jan 21, 2025

jsitarek Dec 23, 2024

jsitarek Dec 23, 2024

jsitarek Dec 23, 2024

codecov bot commented Jan 21, 2025 •

edited

Loading

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

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

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

jsitarek 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

codecov bot commented Jan 21, 2025 • edited Loading

Codecov Report

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

codecov bot commented Jan 21, 2025 •

edited

Loading