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

Introducing Proton Synchrotron #132

Merged
merged 15 commits into from
Feb 10, 2023
Merged

Conversation

dimaniad6
Copy link
Collaborator

We introduce the hadronic process of proton synchrotron radiation, after the changes of Cosimo that added the possibility of a proton particle distribution. The code is basically the same as the electron synchrotron, but instead of the electron mass, the proton mass is used. To do that, we introduced the new mass variable, which can take two values, m_e and m_p. With this PR, we also added an additional distribution of a broken power law with an exponential cut-off. The corresponding pytests are included.

@cosimoNigro
Copy link
Owner

cosimoNigro commented Jan 10, 2023

Hi @IYNBI,

this is a PR, we use this type of thread to discuss new features that are being implemented.
If you have any actual issue with the code please open a new issue (second button in the toolbar right below the repository name).

Cheers

@cosimoNigro
Copy link
Owner

@jsitarek would you have time to take a look at @dimaniad6 PR?

Copy link
Collaborator

@jsitarek jsitarek left a comment

Choose a reason for hiding this comment

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

sorry for a late feedback.
I have two main comments:

  1. synchrotron-self-absorption was introduced also for proton synchrotron radiation, basically by substituting m_e for m_p. The usual formula that is used for SSA is approximate: the slope is derived for the strong absorption boardering case and then crossing of the two boardering cases is used to get the full formula. Maybe it is just my ignorance, but I do not remember ever seeing SSA being considered for protons. The proton-photon interactions have a much smaller cross-section than electron-photon, which should make it much less important process.
  2. a new equivalency was introduced that changes the frequency to energy conversion depending on the particle mass. It might be fine, but I am afraid that it might also produce problems when user calculates both electron and proton processes and wants to put them in the same plot

agnpy/synchrotron/proton_synchrotron.py Outdated Show resolved Hide resolved
agnpy/synchrotron/proton_synchrotron.py Outdated Show resolved Hide resolved
agnpy/utils/conversion.py Show resolved Hide resolved
agnpy/spectra/spectra.py Show resolved Hide resolved
agnpy/spectra/spectra.py Outdated Show resolved Hide resolved
agnpy/synchrotron/proton_synchrotron.py Outdated Show resolved Hide resolved
agnpy/synchrotron/proton_synchrotron.py Outdated Show resolved Hide resolved
agnpy/synchrotron/proton_synchrotron.py Outdated Show resolved Hide resolved
agnpy/synchrotron/synchrotron.py Outdated Show resolved Hide resolved
agnpy/synchrotron/synchrotron.py Outdated Show resolved Hide resolved
agnpy/tests/test_proton_synchrotron.py Show resolved Hide resolved
agnpy/tests/test_spectra.py Show resolved Hide resolved
agnpy/utils/conversion.py Show resolved Hide resolved
@cosimoNigro
Copy link
Owner

cosimoNigro commented Feb 3, 2023

Thanks a lot for your work @dimaniad6!

Some more things, technicals and theoreticals

Technical comments:

  1. Please, the next time that you implement changes to different classes, try to split them in different PRs. Otherwise PRs become very large and difficult to review. For example, the ExpCutoffBrokenPowerLaw implementation could have gone in a separate PR - though I understand in this particular case you implemented it just to validate the proton synchrotoron SED you got from Cerruti;

  2. despite already merging one of your a PRs, you don't appear among the contributors. Also, in this PR, the icon of your profile appears in the first comment, but not alongside the individual commits (as it should be). Here you can see how to correctly set your username and email. This should be sufficient for your contributions to be correctly identified and tagged. You can check your git credentials with git config --list; I suspect the problem might be that the mail is not correctly set.

Theoretical comments:

  1. Is SSA relevant for proton synchrotron?
    I copy-paste part of the content of a mail Julian sent me, I think it perfectly clarifies what are our doubts on SSA for proton synchrotron:

    I am not sure if it is done correctly the same way for protons as for electrons. Moreover if we have both protons and electrons in the blob, electrons might be more efficient at the absorption of proton-synchrotron radiation. I do not
    remember ever seeing SSA of proton synchrotron being considered in the papers, and doing a quick search I cannot find anything either. In normal situation I think it is negligible, because protons couple to photons much weaker than electrons do. I would suggest to simply drop the SSA part for the proton synchrotron.

    Since you introduced it just to reproduce Cerruti's SED, could you ask him to provide you with an SED without SSA? I think it is wise to drop it, if we are not 100% sure it matters and cannot find any other reference in the literature;

  2. about the epsilon equivalencies with m_p, I have already commented in the code.
    @jsitarek, I don't get very well your concern. Users will just plot $\nu$ vs $\nu F_{nu}$, and in this case the x axis is independent on the mass, right? Or are you concerned someone will plot using epsilon?

  3. Our psynch SEDs are - across all the energy range - a factor 0.8 those of Matteo. Could you find out why?
    proton_synch_comparison_PKS

  4. please add some reference for the proton synchrotron calculation (as we did for all the other radiative processes);

@jsitarek
Copy link
Collaborator

jsitarek commented Feb 3, 2023

   Since you introduced it just to reproduce Cerruti's SED, could you ask him to provide you with an SED without SSA? I think it is wise to drop it, if we are not 100% sure it matters and cannot find any other reference in the literature;

I'm pretty sure that in the previous post plot the SSA will not change anything at least > 10^10 Hz.
The drop-off at lower frequency I'm not sure if it comes from SSA, or from the gamma_min of the proton distribution.
Either way, even before asking Matteo to rerun it without SSA I would propose to just rerun the new agnpy code without SSA and see if it still matches. If needed we can limit the range to >10^10 Hz.

2. about the epsilon equivalencies with `m_p`, I have already commented in the code.
   @jsitarek, I don't get very well your concern. Users will just plot ν vs νFnu, and in this case the x axis is independent on the mass, right? Or are you concerned someone will plot using epsilon?

I think this was just a mistake on my side, sorry, it should be fine as it is .

3. Our psynch SEDs are - across all the energy range - a factor 0.8 those of Matteo. Could you find out why?
   ![proton_synch_comparison_PKS](https://user-images.githubusercontent.com/26409711/216488059-60b278dc-53bc-4295-b54c-5cb12acaa9c1.png)

as a quick test you could try to apply finer integration, but probably it will not help.
otherwise to investigate it, it would be good to have a few more curves from Matteo, changing one parameter at the time:

  1. setting z to very small number: 0.01 to avoid problems with differnet cosmology used
  2. checking if the ratio is the same for different doppler beaming, in particular you can try D=1.01 (with Gamma=1.004 or something like this), but making the blob non-relativistic might cause different troubles as well.
  3. running it for a different index of protons and seeing if the ratio keeps the same - this will tell you if the problem is somewhere in the general normalization or in the integrated parts.

@codecov
Copy link

codecov bot commented Feb 10, 2023

Codecov Report

Merging #132 (b4d6097) into master (075933e) will decrease coverage by 0.07%.
The diff coverage is 95.48%.

@@            Coverage Diff             @@
##           master     #132      +/-   ##
==========================================
- Coverage   97.02%   96.95%   -0.07%     
==========================================
  Files          38       40       +2     
  Lines        3094     3256     +162     
==========================================
+ Hits         3002     3157     +155     
- Misses         92       99       +7     
Flag Coverage Δ
unittests 96.95% <95.48%> (-0.07%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
agnpy/tests/test_synchrotron.py 100.00% <ø> (ø)
agnpy/synchrotron/proton_synchrotron.py 85.71% <85.71%> (ø)
agnpy/utils/conversion.py 96.00% <92.30%> (-4.00%) ⬇️
agnpy/spectra/spectra.py 96.29% <92.85%> (+0.01%) ⬆️
agnpy/synchrotron/__init__.py 100.00% <100.00%> (ø)
agnpy/synchrotron/synchrotron.py 94.11% <100.00%> (ø)
agnpy/tests/test_proton_synchrotron.py 100.00% <100.00%> (ø)
agnpy/tests/test_spectra.py 98.88% <100.00%> (+0.18%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@cosimoNigro
Copy link
Owner

Thanks a lot @IlariaViale, @dimaniad6, this is a fantastic new feature of the code!

Can one of you open an issue about the comparison with Cerruti, please?

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