[Kstars-devel] Re: Review Request: This patch fixes the magnitude parameter of observable comets by using a different formula altogether.

[Akarsh Simha]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/101650/#review3946
-----------------------------------------------------------



kstars/skycomponents/cometscomponent.cpp
<http://git.reviewboard.kde.org/r/101650/#comment3222>

    Hmm, why not the if( !ok ) approach? If string conversion fails, we assign a crazy number... Rather than check if it is empty -- it could contain only whitespaces for instance, and this code will assume that you meant H = 0 in that case. Even otherwise, fields.at(9).isEmpty() looks cleaner. I'm not sure if a blank string will cause a result of '0' and ok=true, or a random result and ok=false, though. Need to check the docs.



kstars/skycomponents/cometscomponent.cpp
<http://git.reviewboard.kde.org/r/101650/#comment3221>

    Oh, so I'm the idiot responsible for these atrociously high magnitudes. So if M1, M2, K1, K2 work fine, you should probably get rid of H and G altogether from the code. Also, we need to let Jeamy Lee know.



kstars/skycomponents/cometscomponent.cpp
<http://git.reviewboard.kde.org/r/101650/#comment3223>

    Yes, this is better. Setting it to 101 makes more sense than my stupid setting it to -101. Sure.



kstars/skycomponents/cometscomponent.cpp
<http://git.reviewboard.kde.org/r/101650/#comment3224>

    Again, unnecessary whitespace. git diff will show these red blotches as well (I find them in my patches all the time) if you've enabled color in your .gitrc or whatever.



kstars/skycomponents/cometscomponent.cpp
<http://git.reviewboard.kde.org/r/101650/#comment3226>

    Sure. This looks good. Except I still don't like the fields.at(21) == "", like I said before.



kstars/skycomponents/cometscomponent.cpp
<http://git.reviewboard.kde.org/r/101650/#comment3227>

    Oh wait, I just realized you designed it all to be a CSV. In which case, this is acceptable. Sorry, I thought you were dealing with fixed-width fields. Still, I'd use .isEmpty().



kstars/skycomponents/cometscomponent.cpp
<http://git.reviewboard.kde.org/r/101650/#comment3228>

    Hmm, I wonder what 2400...0.5 is. I know it's not your code.
    



kstars/skycomponents/cometscomponent.cpp
<http://git.reviewboard.kde.org/r/101650/#comment3229>

    kDebug(), can be removed if you like. Not too particular about that.



kstars/skyobjects/kscomet.h
<http://git.reviewboard.kde.org/r/101650/#comment3231>

    Don't you have methods to return M1 and M2? Are those not required?



kstars/skyobjects/kscomet.h
<http://git.reviewboard.kde.org/r/101650/#comment3230>

    Stray whitespace inserted?


- Akarsh


On June 17, 2011, 5:15 a.m., Samikshan Bairagya wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/101650/
> -----------------------------------------------------------
> 
> (Updated June 17, 2011, 5:15 a.m.)
> 
> 
> Review request for KStars.
> 
> 
> Summary
> -------
> 
> The patch is dirty now. Will look into it. This patch fixes the magnitude parameter of observable comets by using a different formula altogether.
> T-Mag = M1 + 5*log10(rearth) + k1*log10(rsun). Comets for which data are not available will have abnormal magnitudes of 99.
> For formula refer: http://ssd.jpl.nasa.gov/horizons.cgi
> 
> Changed CometsComponent::updateDataFile() to store the k1,k2 parameters as well in the comets.dat file
> Changed CometsComponent::loadData() to load M1, M2, K1, K2 parameters as well. Also if M1, M2, K1, K2 data are not available set abnormal values as default.
> Changed KSComet.h and KSComet.cpp to accept M1, M2, K1, K2 parameters as well. Also modified KSComet::findMagnitude() to use the above formula.
> Changed setMag() function in SkyObject.h so that for any magnitudes >=30 abnormally high value(99) is set. 
> 
> 
> Diffs
> -----
> 
>   kstars/dialogs/detaildialog.cpp 3d669ef 
>   kstars/skycomponents/cometscomponent.cpp 4a2851b 
>   kstars/skyobjects/kscomet.h 5a858ee 
>   kstars/skyobjects/kscomet.cpp e665c70 
>   kstars/skyobjects/skyobject.h 6f6a061 
> 
> Diff: http://git.reviewboard.kde.org/r/101650/diff
> 
> 
> Testing
> -------
> 
> Tested for various comets.
> 
> 
> Thanks,
> 
> Samikshan
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.kde.org/pipermail/kstars-devel/attachments/20110617/54b9f032/attachment-0001.htm