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

added eors #24

Merged
merged 15 commits into from
Jul 31, 2018
Merged

added eors #24

merged 15 commits into from
Jul 31, 2018

Conversation

prakharcode
Copy link
Contributor

@giordano any lights on this error?

julia> a = rand(3,3)
3×3 Array{Float64,2}:
 0.286184  0.497562  0.0348385
 0.233051  0.532657  0.925174 
 0.301579  0.295383  0.814692 

julia> equations_of_origins(a, 2)
2.165203821361782

julia> ERFA.eors(a, 2)
0.9459823155896738

@prakharcode prakharcode changed the title [WIP] [WIP] added eors Jul 22, 2018
@codecov-io
Copy link

codecov-io commented Jul 22, 2018

Codecov Report

Merging #24 into master will increase coverage by 0.2%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master      #24     +/-   ##
=========================================
+ Coverage   98.75%   98.95%   +0.2%     
=========================================
  Files           5        5             
  Lines          80       96     +16     
=========================================
+ Hits           79       95     +16     
  Misses          1        1
Impacted Files Coverage Δ
src/AstroBase.jl 98.75% <100%> (+0.31%) ⬆️

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 fadd077...522e4e0. Read the comment docs.

@giordano
Copy link
Member

There are two issues:

  1. ÷ in Julia performs integer division, simply leave / for usual division, just like in the C code
  2. you should know by now what's the main issue 😉

@prakharcode
Copy link
Contributor Author

  1. I tried both / and ÷
  2. Okay, let me have a look again

@giordano
Copy link
Member

giordano commented Jul 22, 2018

In Julia, ÷ and / do very different things.

In C there isn't a native operator to perform integer division between floating point numbers. Instead, division between integer types is integer division, but that's not the case here (all operands in the eors code are double).

src/AstroBase.jl Outdated
@@ -115,4 +115,25 @@ function tio_locator(jd1, jd2)
-47e-6 * t * sec2rad(1)
end

"""
equations_of_origins(rnpb, s)
Copy link
Member

Choose a reason for hiding this comment

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

equation_of_origins

@helgee
Copy link
Member

helgee commented Jul 27, 2018

@prakharcode This is a row-major vs. column-major matrix issue. You will see that if you pass the transpose of your matrix to the function the result will be correct.

@helgee
Copy link
Member

helgee commented Jul 27, 2018

@prakharcode BTW, you could also port the test cases from ERFA so you do not have to come up with your own. See here: https://github.com/liberfa/erfa/blob/master/src/t_erfa_c.c

@helgee
Copy link
Member

helgee commented Jul 27, 2018

@giordano Now I ruined your lesson because I did not read your reply closely 🙈 Sorry 😜

@prakharcode prakharcode changed the title [WIP] added eors added eors Jul 28, 2018
* added

* added gmst06

* added 82

* gmst82

* fixed

* coverage

* Update AstroBase.jl
* pr00

* rebase

* Add doctest

* Update AstroBase.jl
src/AstroBase.jl Outdated
zs = -x
p = (rnpb[1, 1] * xs) + (rnpb[2, 1] * ys) + (rnpb[3, 1] * zs)
q = (rnpb[1, 2] * xs) + (rnpb[2, 2] * ys) + (rnpb[3, 2] * zs)
((p != 0) || (q != 0)) ? s - atan2(q, p) : s
Copy link
Member

Choose a reason for hiding this comment

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

Parens are unnecessary.

Copy link
Member

Choose a reason for hiding this comment

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

atan(q, p) see JuliaLang/julia#27248

src/AstroBase.jl Outdated
sec2rad((xpr + (xyls[1] + xypl[1]) / 1e6)), sec2rad(ypr + (xyls[2] + xypl[2]) / 1e6)
end

"""
equations_of_origins(rnpb, s)
Copy link
Member

Choose a reason for hiding this comment

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

equation_of_origins

src/AstroBase.jl Outdated
function equation_of_origins(rnpb, s)
x = rnpb[1, 3]
ax = x / (1.0 + rnpb[3, 3])
xs = 1.0 - (ax * x)
Copy link
Member

Choose a reason for hiding this comment

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

Actually, all parens beyond this point should be removed (except for the function call) 😂 https://en.wikipedia.org/wiki/Order_of_operations#Mnemonics 😜

@helgee
Copy link
Member

helgee commented Jul 31, 2018

There were some unrelated changes (deleted lines etc.) but I have fixed them myself.

@helgee helgee merged commit 29064d4 into JuliaAstro:master Jul 31, 2018
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