-
Notifications
You must be signed in to change notification settings - Fork 21
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
Add - Tics Function #8
Conversation
* TODO.md: remove 'tics' from list. * docs/src/ref.md: add "tics" entry to the manual. * src/utils.jl: Add tics entry to "utils". * src/tics.jl: Contains tics function. * test/util-tests.jl: Include tests Note - The test and examples values are random dummy values, as I couldn't find any proper example sets to use.
* src/tics.jl: Re-added spacing in the function info section for readibility, which had somehow gotten lost in transition.
Codecov Report
@@ Coverage Diff @@
## master #8 +/- ##
==========================================
+ Coverage 99.78% 99.79% +0.01%
==========================================
Files 57 58 +1
Lines 943 999 +56
==========================================
+ Hits 941 997 +56
Misses 2 2
Continue to review full report at Codecov.
|
src/tics.jl
Outdated
# => (0.66,2.0) | ||
tics(30, 60, 12, 2, true) | ||
# => (2.75,30.0) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You missed to close the ```
block ;-)
Thanks again. Some comments:
|
src/tics.jl
Outdated
|
||
numtics = numx/ticsize | ||
if ra | ||
mul = 4.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here, and in the other assignments below where only numbers are involved, you can write T(4)
instead of 4.0
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apologies, I didn't realise the significance of T(number) earlier.
Added T() to all assignment areas.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't worry, no problem, sorry if I wasn't clear enough ;-)
src/tics.jl
Outdated
elseif incr >= 0.5 | ||
incr = 1.0 | ||
elseif incr >= 0.25 | ||
incr = 0.5 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In case of decimal numbers, use T(1//2)
.
src/tics.jl
Outdated
tics(radec_min::Real, radec_max::Real, numx::Real, | ||
ticsize::Real, ra::Bool=false) = | ||
_tics(promote(float(radec_min), float(radec_max), float(numx), | ||
float(ticsize))..., ra) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a nitpick: please insert a line break at the end of the last line, and indent it.
* src/tics.jl: Removed float for numeric literals Fixed indentation, added promt pasting * test/utils-tests.jl: In brief, brute forced for test coverage Potential Problem: The function has a unstable return type. Possible annotation?
Committed fixes. As always, thanks for the help!
The commit reflects the same, however running @code_warntype states that the function now has a unstable return type (Union{Int64, Rational{Int64}}) . The style guide recommends that I should annotate it. Thoughts?
I noticed the prompt pasting in a few of the files like month_cnv.jl, however they weren't working for me because apparently I was still running Julia 0.5. Realised that after you pointed out the version condition. Another query, should we implement the feature of prompt pasting in all the existing function files? (As a minor todo/issue) |
As I suggested in the comments to the code, you can use |
* src/tics.jl: Implemented T(number) for accuracy and type stabiltiy * test/utils-tests.jl: Edited tests to accommodate changes in tics.jl
Note - The test and examples values are random dummy values,
as I couldn't find any proper example sets to use.