Photon power law #103

settwi · 2023-06-05T03:08:48Z

PR

PR Description

Adds photon space broken power law.

The broken power law bins flux properly and uses an analytical antiderivative of a power law to do so.
The documentation is (mostly?) in line with other stuff I've seen in sunxspex so far.
Needs more tests!

Fixes #101

codecov-commenter · 2023-06-05T03:12:19Z

Codecov Report

Patch coverage: 97.77% and project coverage change: +0.60% 🎉

Comparison is base (c40792c) 55.06% compared to head (9da8014) 55.66%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #103      +/-   ##
==========================================
+ Coverage   55.06%   55.66%   +0.60%     
==========================================
  Files          19       20       +1     
  Lines        3122     3167      +45     
==========================================
+ Hits         1719     1763      +44     
- Misses       1403     1404       +1

Files Changed	Coverage Δ
sunxspex/photon_power_law.py	`97.77% <97.77%> (ø)`

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

settwi · 2023-06-05T03:25:05Z

The Python 3.7 checks will always fail because I'm using the walrus operator

DanRyanIrish

Hi @settwi. This looks good. Could you convert the "plot test" into an actual test where you check the output values?

settwi · 2023-06-05T12:32:26Z

Yes! I'll work on that today. Thank you

sunxspex/photon_power_law.py

sunxspex/tests/test_photon_power_law.py

samaloney

Nice additions @settwi comments below are more of a discussion for us all have rather than you to address/fix

Would a power_law_binned_flux be useful I know it can be achieved using the break energy but that's kinda opaque
Type annotations I'm a fan but we should probably make a decision as package/group
Is the root folder the correct place for these kind of functions
There are similar functions but for electrons in the thick target models could they be combined/integrated - less code is better than more code

sunxspex/photon_power_law.py

changelog/103.feature.rst

sunxspex/photon_power_law.py

settwi · 2023-06-06T13:53:32Z

@samaloney thanks for the feedback! I'll address the style and testing feedback hopefully today and push up the changes.

settwi · 2023-06-09T19:13:42Z

I squashed a couple bugs, adjusted the formatting, and tried to address the testing concerns that folks mentioned. Please take a look and let me know if other things need to change. Thanks!

PS: i am stuck trying to get Sphinx to properly build. weird indentation errors. ugh

@samaloney @DanRyanIrish @KriSun95

sunxspex/photon_power_law.py

changelog/103.feature.rst

sunxspex/photon_power_law.py

Co-authored-by: Shane Maloney <maloneys@tcd.ie>

settwi · 2023-06-15T13:37:23Z

Thanks for the fixes @samaloney I am not sure why but I couldn't get my local Sphinx to build the documentation without errors

Are we good to merge?

samaloney · 2023-06-15T13:56:10Z

No worries a fresh install in a clean virtual env e.g. pip install -e .[dev] and it should just work but can be finicky.

As far as code is concerned I think the PR is good to go but I think there some 'meta' issues that need to be discussed I had suggest at the proposed monthly call?

settwi · 2023-06-21T14:39:04Z

@DanRyanIrish and @samaloney can we say your revisions have been satisfied and merge the PR?

KriSun95 · 2023-07-06T17:01:45Z

I think this PR might be ready to merge. Should we add it in?

settwi · 2023-07-12T03:56:13Z

@samaloney @DanRyanIrish could one of you approve the PR if you think it's good to go?

DanRyanIrish

This looks OK to me. A couple general style points though.

I think it's good practice to have readable variable names. In the tests and main code there are several 2- and 3-letter variable names that are not descriptive, making the code harder to understand.

Similarly, it's good practice name functions with verbs. That way you can tell it's a function/method from its name., e.g. power_law_binned_flux -> calculate_power_law_binned_flux

sunxspex/tests/test_photon_power_law.py

DanRyanIrish · 2023-07-12T21:38:49Z

sunxspex/photon_power_law.py

+import numpy as np
+
+import astropy.units as u


Suggested change

import numpy as np

import astropy.units as u

import astropy.units as u

import numpy as np

See section on ordering imports in PEP 8

I actually ran autopep8 on this file multiple times :p

It looks like this example given in the PEP8 "Imports" section shows that the foo.bar package should be imported second?

import myclass import foo.bar.yourclass

settwi · 2023-07-12T22:18:34Z

Sure, I can update the variables.

hayesla

this looks great @settwi!

changelog/103.feature.rst

hayesla · 2023-07-17T09:05:32Z

sunxspex/photon_power_law.py

+def compute_broken_power_law(
+    energy_edges: u.keV,
+    reference_energy: u.keV,
+    reference_flux: PHOTON_RATE_UNIT,


if there a reason this is called "reference_flux" rather that "normalization"?

i think it's an artifact from when the function had a slightly different implementation. i'll update the name!

hayesla · 2023-07-17T09:06:07Z

sunxspex/photon_power_law.py

+    reference_energy : `astropy.units.Quantity`
+        Energy at which the normalization is applied, i.e. :math:`E_0`.
+    reference_flux : `astropy.units.Quantity`
+        Normalization flux for the photon power law.


I would also add another description here for what this normalisation is

DanRyanIrish

Hi @settwi. Sorry for taking so long to look at this. It also also looks good to me, although I notice a couple of the tests are failing. I've set the tests rerunning. Once they are passing and @hayesla's comments are addressed, we should get this merged!

settwi · 2023-07-24T17:02:30Z

hey @DanRyanIrish @hayesla i will implement Laura's comments. thanks for checking in.
as for the tests: i think some of them are not my fault that they don't pass. for one of them there is something messed up on the CI side i believe. the ones that i can fix i will fix though.
thanks!

Co-authored-by: Laura Hayes <hayesla@tcd.ie>

settwi · 2023-08-09T14:43:41Z

@hayesla and @DanRyanIrish i've implemented Laura's comments and i think things are good to go. i know some tests are failing but i think most of them are out of my hands...

settwi · 2023-09-03T15:33:02Z

@samaloney @DanRyanIrish could you mark the pending changes as approved?

KriSun95 · 2023-09-19T16:57:24Z

@settwi reminded me of a conversation with @DanRyanIrish at SPD that this was OK to be accepted from his point of view so, before any larger changes are made to the repo, I'm going to merge this PR. Any other changes down the line can be addressed in the refactoring.

Conversation at SPD with Dan made clear he was happy to accept the PR but physically couldn't at the time.

settwi added 2 commits January 10, 2023 16:42

photon broken power law

fc928bc

pep8 and changelog

f5a594a

settwi added 3 commits June 4, 2023 22:12

undo weird uncommenting on __init__

4f1d225

pep8 -> test

0344ef6

isort fix

427dd32

DanRyanIrish previously requested changes Jun 5, 2023

View reviewed changes

KriSun95 reviewed Jun 5, 2023

View reviewed changes

sunxspex/photon_power_law.py Outdated Show resolved Hide resolved

KriSun95 reviewed Jun 5, 2023

View reviewed changes

sunxspex/tests/test_photon_power_law.py Outdated Show resolved Hide resolved

samaloney requested changes Jun 6, 2023

View reviewed changes

settwi added 11 commits June 8, 2023 10:50

fix changelog formatting

549162f

add photon power law to docs

241779e

replace assert with if + ValueError

4fb7a90

add some tests & fix integration bug)

99a6570

add single power law

1da5697

more power law tests

b4ed6f3

docstrings and formatting

1c902a0

clean up tests formatting

ad096f7

isort and autopep

d5b728c

fix math formatting

b890f69

fix docstrings for sphinx

467c6b6

samaloney reviewed Jun 12, 2023

View reviewed changes

sunxspex/photon_power_law.py Outdated Show resolved Hide resolved

samaloney reviewed Jun 12, 2023

View reviewed changes

changelog/103.feature.rst Outdated Show resolved Hide resolved

samaloney reviewed Jun 12, 2023

View reviewed changes

sunxspex/photon_power_law.py Outdated Show resolved Hide resolved

settwi and others added 2 commits June 14, 2023 16:54

Update changelog/103.feature.rst

ab9d9d7

Co-authored-by: Shane Maloney <maloneys@tcd.ie>

Update sunxspex/photon_power_law.py

cc7a73f

Co-authored-by: Shane Maloney <maloneys@tcd.ie>

settwi and others added 3 commits June 14, 2023 17:16

hopefully finally fix math errors and doc formatting

d60273d

Fix docstring math equations

22730dc

Fix precommit issue

de15697

KriSun95 approved these changes Jul 6, 2023

View reviewed changes

KriSun95 requested review from samaloney and DanRyanIrish July 6, 2023 17:23

Merge branch 'sunpy:master' into photon-power-law

5c81fba

DanRyanIrish reviewed Jul 12, 2023

View reviewed changes

DanRyanIrish style changes :)

94d5f31

hayesla approved these changes Jul 17, 2023

View reviewed changes

DanRyanIrish reviewed Jul 24, 2023

View reviewed changes

settwi and others added 3 commits July 24, 2023 12:03

Update changelog/103.feature.rst

0d74a26

Co-authored-by: Laura Hayes <hayesla@tcd.ie>

rename 'reference' to 'normalization'

a15742f

clarify normalization flux parameter docstring

9da8014

settwi force-pushed the photon-power-law branch from 0f10b6f to 9da8014 Compare August 9, 2023 14:42

samaloney approved these changes Sep 7, 2023

View reviewed changes

settwi requested a review from DanRyanIrish September 11, 2023 18:00

KriSun95 merged commit 8155913 into sunpy:master Sep 19, 2023
5 of 10 checks passed

settwi deleted the photon-power-law branch September 19, 2023 17:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Photon power law #103

Photon power law #103

settwi commented Jun 5, 2023

codecov-commenter commented Jun 5, 2023 •

edited

settwi commented Jun 5, 2023

DanRyanIrish left a comment

settwi commented Jun 5, 2023

samaloney left a comment

settwi commented Jun 6, 2023

settwi commented Jun 9, 2023

settwi commented Jun 15, 2023 •

edited

samaloney commented Jun 15, 2023

settwi commented Jun 21, 2023

KriSun95 commented Jul 6, 2023

settwi commented Jul 12, 2023

DanRyanIrish left a comment

DanRyanIrish Jul 12, 2023

settwi Jul 14, 2023

settwi commented Jul 12, 2023 via email •

edited

hayesla left a comment

hayesla Jul 17, 2023

settwi Jul 24, 2023

hayesla Jul 17, 2023

DanRyanIrish left a comment

settwi commented Jul 24, 2023

settwi commented Aug 9, 2023

settwi commented Sep 3, 2023

KriSun95 commented Sep 19, 2023

Photon power law #103

Photon power law #103

Conversation

settwi commented Jun 5, 2023

PR Description

codecov-commenter commented Jun 5, 2023 • edited

Codecov Report

settwi commented Jun 5, 2023

DanRyanIrish left a comment

Choose a reason for hiding this comment

settwi commented Jun 5, 2023

samaloney left a comment

Choose a reason for hiding this comment

settwi commented Jun 6, 2023

settwi commented Jun 9, 2023

settwi commented Jun 15, 2023 • edited

samaloney commented Jun 15, 2023

settwi commented Jun 21, 2023

KriSun95 commented Jul 6, 2023

settwi commented Jul 12, 2023

DanRyanIrish left a comment

Choose a reason for hiding this comment

DanRyanIrish Jul 12, 2023

Choose a reason for hiding this comment

settwi Jul 14, 2023

Choose a reason for hiding this comment

settwi commented Jul 12, 2023 via email • edited

hayesla left a comment

Choose a reason for hiding this comment

hayesla Jul 17, 2023

Choose a reason for hiding this comment

settwi Jul 24, 2023

Choose a reason for hiding this comment

hayesla Jul 17, 2023

Choose a reason for hiding this comment

DanRyanIrish left a comment

Choose a reason for hiding this comment

settwi commented Jul 24, 2023

settwi commented Aug 9, 2023

settwi commented Sep 3, 2023

KriSun95 commented Sep 19, 2023

codecov-commenter commented Jun 5, 2023 •

edited

settwi commented Jun 15, 2023 •

edited

settwi commented Jul 12, 2023 via email •

edited