Add - Ordinal Function

[TestSubjector]

Translated the simple Ordinal IDL AstroLib procedure, which returns the English equivalent of ordinal numbers, i.e. '1st','2nd' etc.
PR for the same.

Note - Used if-else statements (unlike cases in the original IDL AstroLib procedure) while translating as Julia lacks a C-style switch statement.

[coveralls]

Coverage decreased (-1.4%) to 98.411% when pulling 34b7be9 on TestSubjector:ordinal into 8648b7b on JuliaAstro:master.

[codecov-io]

Codecov Report

Merging #7 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master       #7      +/-   ##
==========================================
+ Coverage   99.78%   99.78%   +<.01%     
==========================================
  Files          56       57       +1     
  Lines         931      943      +12     
==========================================
+ Hits          929      941      +12     
  Misses          2        2

Impacted Files	Coverage Δ
src/ordinal.jl	100% <100%> (ø)

---

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 8648b7b...6644263. Read the comment docs.

[giordano]

Thank you so much for your contribution! However I have some comments:

• an important point most Julia newcomers aren't familiar with is type-stability: in order to produce efficient code you must be sure that a variable will always keep the same type, see http://docs.julialang.org/en/latest/manual/performance-tips.html#Write-\ Your function takes a num argument that is any subtype of Real than is converted to Int. You can avoid this issue by assigning this conversion to a. Don't worry, I think that everyone wrote many type-unstable functions before doing it right ;-) Use @code_warntype, it'll be your best friend at the beginning of your Julia trip.

• actually I'm not sure it's useful to take any Real, probably an Integer suffices here (and the conversion mentioned in the previous point is no more needed, but always mind type-instabilities). I see that this is how the IDL function works, but try to be smarter than those functions.

• another performance tip is to avoid string interpolation: return string(num) * suffix instead.

• please add a test set for the new function, in order to test every possible situation. It's really important in order to ensure consistency of the results and to avoid that bugs will be accidentally introduced in the future. I strive to keep code coverage as close as possible to 100%.

• In addition, with a test you would notice that the function isn't actually included in the library, nor exported: see the file src/utils.jl. Admittedly, I'm not particularly proud of the code is organized right now, it may change in the future, but this is how it works now.

Another, minor, stylistic comment: please remove trailing whitespaces.

I think this is enough. Are you able to fix everything y yourself? Anyway, that was a good start!

[TestSubjector]

Thank you for the detailed reply.

• You're correct, I wasn't aware of the type-stability and the performance tips. I'll be sure to go through that.

• I kept the conversion so as to keep it similar to the structure of the IDL procedure, which I assumed (incorrectly) was a rigid rule to follow.

• I noticed the purpose of the src/utils.jl file, but I didn't want to modify it until I got a review on this commit.

I'll make the necessary changes including the test set. Should I add those changes as further commits on this branch or create a new PR?
Thanks again for the help.

[giordano]

I kept the conversion so as to keep it similar to the structure of the IDL procedure, which I assumed (incorrectly) was a rigid rule to follow.

No, it's not necessary to closely follow the original implementation. Well, I mostly did so, but the important is to obtain exact results paying attention to performance.

Should I add those changes as further commits on this branch or create a new PR?

Just keep editing your branch (you can either amend the commit or push new changes), this PR will be updated, no need to open a new one.

Thanks!

[giordano]

Only another comment: Int is not the same thing as Integer, see http://docs.julialang.org/en/latest/manual/integers-and-floating-point-numbers.html. Int is a concrete type that defaults to Int32 in 32-bit systems and Int64 in 64-bit systems, but in general we want the functions to be as loosely typed as possible, so abstract types should be preferred ;-)

[giordano]

I installed a few minor changes related to the organization of the code and merged the PR. Thanks!

[TestSubjector]

Thanks, I'll make a note of the changes for future PRs