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

Fixing deprecations in previously not included functions #3

Merged
merged 4 commits into from
Apr 22, 2018

Conversation

Brinky5678
Copy link

Dear Maintainers,

Within the "Spice.jl" module definition, I included the files "constants.jl", "math.jl", "pck.jl", and "spk.jl". I required the use of the functions already created in these files, yet were previously not exported. Additionally, some deprecation warnings were fixed in those files regarding array definitions.

Please consider my changes.

With kind regards,

Dennis Brinkman

@coveralls
Copy link

Pull Request Test Coverage Report for Build 83

  • 0 of 7 (0.0%) changed or added relevant lines in 3 files are covered.
  • 2 unchanged lines in 1 file lost coverage.
  • Overall coverage decreased (-0.4%) to 61.667%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/math.jl 0 1 0.0%
src/pck.jl 0 3 0.0%
src/spk.jl 0 3 0.0%
Files with Coverage Reduction New Missed Lines %
src/b.jl 2 95.45%
Totals Coverage Status
Change from base Build 82: -0.4%
Covered Lines: 148
Relevant Lines: 240

💛 - Coveralls

@coveralls
Copy link

coveralls commented Apr 15, 2018

Pull Request Test Coverage Report for Build 85

  • 38 of 54 (70.37%) changed or added relevant lines in 6 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+25.7%) to 87.773%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/m.jl 7 8 87.5%
src/s.jl 21 36 58.33%
Totals Coverage Status
Change from base Build 82: 25.7%
Covered Lines: 201
Relevant Lines: 229

💛 - Coveralls

@codecov-io
Copy link

codecov-io commented Apr 15, 2018

Codecov Report

Merging #3 into master will increase coverage by 25.68%.
The diff coverage is 68%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master       #3       +/-   ##
===========================================
+ Coverage   62.08%   87.77%   +25.68%     
===========================================
  Files          18       15        -3     
  Lines         240      229       -11     
===========================================
+ Hits          149      201       +52     
+ Misses         91       28       -63
Impacted Files Coverage Δ
src/b.jl 100% <ø> (ø) ⬆️
src/k.jl 100% <ø> (+100%) ⬆️
src/SPICE.jl 90.9% <ø> (ø) ⬆️
src/t.jl 100% <100%> (+100%) ⬆️
src/c.jl 93.1% <100%> (+0.24%) ⬆️
src/p.jl 100% <100%> (ø)
src/s.jl 52.5% <58.33%> (+52.5%) ⬆️
src/m.jl 87.5% <87.5%> (ø)
... and 3 more

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 e37f139...c7fec5e. Read the comment docs.

@helgee
Copy link
Member

helgee commented Apr 16, 2018

Thank you for your contribution! Could I bother you to move the functions you need to the respective alphabetical file? I orginally wanted to group them by functionality but lost track due to the large number of functions in SPICE.

If you don't have the time I will merge and move them myself.

@Brinky5678
Copy link
Author

No Problem. Somewhere this week I will move everything to be in the correct alphabetical order. Additionally, I will also include some tests of the functions to improve the code coverage.

@Brinky5678
Copy link
Author

Updated the branch, including additional tests. Not sure about the codecov/patch, maybe I don't fully understand how that features works yet.

matrix = [1. 1. 1.
2. 3. 4.]
vecgood = [1., 2., 3.]
@test mxvg(matrix, vecgood) == [6.,20.]
Copy link
Member

Choose a reason for hiding this comment

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

The problem is that CSPICE expects the matrix to be in row-major order whereas Julia uses column-major order.
We need to pass the transpose of the input matrix to the C code, e.g.:

function mxvg(m1, v2)
    lm1, lm2 = size(m1)
    lv = length(v2)
    if lm2 != lv
        error("Dimension mismatch.")
    end
    vout = Array{Float64}(lm1)
    ccall((:mxvg_c, libcspice), Void, (Ptr{Float64}, Ptr{Float64}, Cint, Cint, Ptr{Float64}),
    #= use transpose =# m1', v2, lm1, lm2, vout)
    handleerror()
    return vout
end

@Brinky5678
Copy link
Author

That was indeed the problem! It is working now.

@helgee
Copy link
Member

helgee commented Apr 18, 2018

Please activate the test and then this is good to go 👍 Thank you!

(BTW: You might want to add the email address you use for your Git commits to your GitHub account. This way they become associated to your account and become clickable in the interface.)

@helgee helgee merged commit d3e542e into JuliaAstro:master Apr 22, 2018
@Brinky5678 Brinky5678 deleted the Fixing-Deprecations branch April 22, 2018 09:36
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