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

Adstring outputs wrong string with negative zero #57

Closed
G-Francio opened this issue Feb 26, 2020 · 5 comments · Fixed by #74
Closed

Adstring outputs wrong string with negative zero #57

G-Francio opened this issue Feb 26, 2020 · 5 comments · Fixed by #74

Comments

@G-Francio
Copy link

Adstring outputs the wrong string when dec starts with negative zero (i.e. -0.5), while radec converts correctly.

>> adstring(15, -0.5)
" 01 00 00.0  +00 30 00"

>> radec(15, -0.5)
(1.0, 0.0, 0.0, -0.0, 30.0, 0.0)

The bug seems to be this line in adstring.jl:

dec_string = @sprintf("%+03.2d %02d %s", dec_deg, dec_min, dec_sec_string)

which ignores the negative zero.
A possible fix (although, since I'm a beginner, probably not a reliable one) could be:
dec_string = (dec > 0 ? "+" : "-") * @sprintf("%02.2d %02d %s", abs(dec_deg), dec_min, dec_sec_string)

@Aman-Pandey-afk
Copy link
Contributor

I want to work on this issue, how should I get started? I already have build AstroLib and tried to modify according to the suggestion mentioned (though rebuilding the pkg doesn't reflect it).

@giordano
Copy link
Member

I don't know what you mean by "I have already build AstroLib", but the best way to work on a Julia package is to develop it. In the Julia REPL, type ] to enter Pkg REPL mode, then run the command

develop AstroLib

or in short

dev AstroLib

This will have created a git clone of this repository under your .julia directory, in particular in .julia/dev/AstroLib. Then, you can edit the source code of the package in this directory and when you restart the Julia session you'll see the changes taking effect (in the same environment as where you developed the package). Note that you can avoid restarting the Julia sessions if you're using Revise.jl.

Once you're done with the changes, you can submit a pull request here. Note that it's always good practice to add new tests in a pull request which is fixing a bug. Let me know if you need more guidance about how to open a pull request.

@Aman-Pandey-afk
Copy link
Contributor

I looked into the problem and found that the above suggestion by G-Francio requires just int casting to produce the correct output. The first one is his output and second one is mine after modifying abs(dec_deg) to Int(abs(dec_deg)). Condition dec>0 also needed modification to dec>=0

julia> adstring(25, -0.6)
" 01 40 00.0  - 0 36 00"

julia> adstring(25, -0.6)
" 01 40 00.0  -00 36 00"

The modified line:
dec_string = (dec >= 0 ? "+" : "-") * @sprintf("%02.2d %02d %s", Int(abs(dec_deg)), Int(dec_min), dec_sec_string)

I have tried it for several tests, it is good looking for every case. Also, we can use just
dec_string = @sprintf("%+02.2d %02d %s", dec_deg, Int(dec_min), dec_sec_string)
This outputs like following (which maybe undesirable) :

adstring(25, -0.6)
" 01 40 00.0  -0 36 00"

It maybe a help to know if there is any guidelines for pull requests, or just I create a new branch (Pushing the AstroLib directory inside .julia/dev) and make a pr like usual.

@Aman-Pandey-afk
Copy link
Contributor

@giordano it would be great of you if you could provide some feedback on this and also contribution practices you follow. I would be glad to work on any other issue also, if you could point it out.

@giordano
Copy link
Member

It maybe a help to know if there is any guidelines for pull requests, or just I create a new branch (Pushing the AstroLib directory inside .julia/dev) and make a pr like usual.

Yes, that's absolutely fine. We can discuss the details of the implementation in the pull request

@giordano giordano linked a pull request Apr 4, 2022 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants