Skip to content

Added M14 model #33

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

Merged
merged 9 commits into from
Jul 20, 2020
Merged

Added M14 model #33

merged 9 commits into from
Jul 20, 2020

Conversation

icweaver
Copy link
Member

@icweaver icweaver mentioned this pull request Jul 16, 2020
10 tasks
@codecov
Copy link

codecov bot commented Jul 16, 2020

Codecov Report

Merging #33 into master will increase coverage by 0.03%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #33      +/-   ##
==========================================
+ Coverage   99.52%   99.55%   +0.03%     
==========================================
  Files           4        4              
  Lines         209      224      +15     
==========================================
+ Hits          208      223      +15     
  Misses          1        1              
Impacted Files Coverage Δ
src/DustExtinction.jl 100.00% <ø> (ø)
src/color_laws.jl 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2520754...d17f633. Read the comment docs.

@icweaver
Copy link
Member Author

icweaver commented Jul 18, 2020

5cdb724

Seems to be failing to build Dierckx.jl in Julia 1.0 on FreeBSD and Julia 1.4 in Linux Xenial?

https://travis-ci.org/github/JuliaAstro/DustExtinction.jl/jobs/709308305#L300

Oh, may have just been spamming TravisCI too much.

Yep, all good

Comment on lines 557 to 558
a = ai * (x < m14_xi1) + m14_av * ((x >= m14_xi1) & (x < m14_xi3))
b = bi * (x < m14_xi1) + m14_bv * ((x >= m14_xi1) & (x < m14_xi3))
Copy link
Member

Choose a reason for hiding this comment

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

I know this is perfectly legal and very efficient, but I find these lines hard to read, it took me a while to realise that this should be equivalent to something like

a = zero(m14_av)
b = zero(m14_bv)
if x < m14_xi1
    # Infrared
    a = 0.574 * x^1.61
    b = -0.527 * x^1.61
elseif x < m14_xi3
    # Optical
    a = m14_a_spl(x)
    b = m14_b_spl(x)
end

which also saves you some computations above. An alternative would be

a = x < m14_xi1 ? ai : x < m14_xi3 ? m14_av : zero(m14_av)
b = x < m14_xi1 ? bi : x < m14_xi3 ? m14_bv : zero(m14_bv)

Copy link
Member Author

Choose a reason for hiding this comment

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

Wow, that reads so much better, thanks! Just changed it c042984

Copy link
Member

@mileslucas mileslucas left a comment

Choose a reason for hiding this comment

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

Seems like some unneccessary comments can be removed, otherwise rebase and I can release as part of v0.10

@giordano
Copy link
Member

A comment about version numbers: new features in the 0.x series don't require a minor version bump. Unless we break the API, we can simply bump the patch version, avoiding releasing a breaking version each time, creating annoying headaches to anyone that cares about version compatibilities.

@icweaver
Copy link
Member Author

Seems like some unneccessary comments can be removed, otherwise rebase and I can release as part of v0.10

Ok, should be good to go

@mileslucas mileslucas merged commit 01be6da into JuliaAstro:master Jul 20, 2020
@icweaver icweaver deleted the M14 branch July 20, 2020 04:01
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