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

BeamedPointSource model added #150

Merged
merged 10 commits into from
Aug 25, 2023

Conversation

wiktoriatarnopolska
Copy link
Contributor

@wiktoriatarnopolska wiktoriatarnopolska commented Aug 25, 2023

BeamedPointSource model added to src/corona/models/ + added docstrings

@fjebaker
Copy link
Member

Don't forget to include the file here:

include("models/lamp-post.jl")

Copy link
Member

@fjebaker fjebaker left a comment

Choose a reason for hiding this comment

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

Looks good, just need a test case to make sure we don't break behaviour in the future! :)

`include("models/moving-source.jl")
@wiktoriatarnopolska
Copy link
Contributor Author

include-d the new model. will add a test case now

Copy link
Member

@fjebaker fjebaker left a comment

Choose a reason for hiding this comment

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

Cool stuff!

Comment on lines 30 to 46
function Gradus.sample_position_velocity(m::AbstractMetric, model::BeamedPointSource)
x = SVector{4}(0, model.r, 1e-4, 0)
g = metric_components(m, SVector(x[2], x[3]))
v̄ = SVector(1, __BeamedPointSource.drdt(g, model.β), 0, 0)
v = Gradus.constrain_normalize(m, x, v̄; μ = 1)
x, v
end

# since we have axis-symmetry, exploit this method for much faster computation
Gradus.emissivity_profile(
::Nothing,
m::AbstractMetric,
d::AbstractAccretionDisc,
model::BeamedPointSource,
spec::Gradus.AbstractCoronalSpectrum;
kwargs...,
) = Gradus._point_source_symmetric_emissivity_profile(m, d, model, spec; kwargs...)
Copy link
Member

@fjebaker fjebaker Aug 25, 2023

Choose a reason for hiding this comment

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

Don't need to prefix Gradus. here, especially not for AbstractCoronalSpectrum, since we're already in the Gradus module namespace.

spec::Gradus.AbstractCoronalSpectrum;
kwargs...,
) = Gradus._point_source_symmetric_emissivity_profile(m, d, model, spec; kwargs...)

Copy link
Member

@fjebaker fjebaker Aug 25, 2023

Choose a reason for hiding this comment

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

Also should have

export BeamedPointSource

so that users can just do BeamedPointSource instead of Gradus.BeamedPointSource.

comparison of lamp post model and moving source model with velocity set to zero -- test
@codecov-commenter
Copy link

codecov-commenter commented Aug 25, 2023

Codecov Report

Merging #150 (25cad1c) into main (65b92e9) will increase coverage by 0.86%.
Report is 6 commits behind head on main.
The diff coverage is 90.00%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

@@            Coverage Diff             @@
##             main     #150      +/-   ##
==========================================
+ Coverage   68.07%   68.94%   +0.86%     
==========================================
  Files          59       60       +1     
  Lines        2622     2634      +12     
==========================================
+ Hits         1785     1816      +31     
+ Misses        837      818      -19     
Files Changed Coverage Δ
src/corona/corona-models.jl 83.33% <ø> (ø)
src/corona/models/moving-source.jl 90.00% <90.00%> (ø)

... and 7 files with indirect coverage changes

Copy link
Member

@fjebaker fjebaker left a comment

Choose a reason for hiding this comment

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

Test is good as a consistency check. At some point will want to add a test for the cases where $\beta &gt; 0$, but we'll check the model before we get carried away with that.

You still need to include this test in the test suite: add a line just after here

include("unit/emissivity.jl")

to include this file, else it wont run.

Comment on lines 35 to 36
plot(profile0, label = "lamp post", linecolor = "magenta")
plot!(profile1, label = "moving src β = 0", linecolor = "royalblue4")
Copy link
Member

Choose a reason for hiding this comment

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

Remove plotting code from tests.

d,
model0,
n_samples = 100_000,
#sampler = EvenSampler(BothHemispheres(), GoldenSpiralGenerator())
Copy link
Member

Choose a reason for hiding this comment

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

Remove commented code.

test/runtests.jl Outdated
@@ -23,6 +23,7 @@ end
include("unit/coronal-beaming.jl")
include("smoke-tests/coronal-spectra.jl")
include("unit/emissivity.jl")
include("test/beamedpointsource-test.jl")
Copy link
Member

Choose a reason for hiding this comment

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

Should just be

include("beamedpointsource-test.jl")

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

Copy link
Member

@fjebaker fjebaker left a comment

Choose a reason for hiding this comment

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

LGTM

@fjebaker fjebaker merged commit 0816528 into astro-group-bristol:main Aug 25, 2023
1 check passed
@fjebaker fjebaker mentioned this pull request Sep 6, 2023
13 tasks
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.

3 participants