Stellarium / stellarium

Stellarium is a free GPL software which renders realistic skies in real time with OpenGL. It is available for Linux/Unix, Windows and macOS. With Stellarium, you really see what you can see with your eyes, binoculars or a small telescope.
https://stellarium.org
GNU General Public License v2.0
7.83k stars 832 forks source link

custom comet magnitude #2932

Open axd1967 opened 1 year ago

axd1967 commented 1 year ago

Is your feature request related to a problem? Please describe.

Stellarium implements a standard magnitude algorithm using "magic numbers" 5 and 2.5 :

https://github.com/Stellarium/stellarium/blob/239b713bdc554e5b6bc30841ad7b0040c882e1dd/src/core/modules/Comet.cpp#L237-L243

Comets seem to follow individual values. Example taken from aerith:

Describe the solution you'd like

A dialog that allows one or more of following functions

Describe alternatives you've considered A clear and concise description of any alternative solutions or features you've considered.

Additional context

  1. Some refactoring elsewhere is probably also needed, to deal with code copy-pasta that deals with magnitude: searching for the string log10 returns several code lines that look suspiciously similar.
  2. As a very minimum, all literal data (2.5, 5, ...) should be replaced by constants, especially when those constants are used in several places. (Note: 5 in 5 log (d) in the formula is a generally accepted constant, nevertheless it could be named, e.g. observerDistance_fading.) more magic constants: https://github.com/Stellarium/stellarium/blob/c85aa797016b9e0c339cfff695d6eae13a42370b/src/core/modules/SolarSystem.cpp#L1053-L1054
  3. A similar formula is used for asteroid magnitudes.

See also

gzotti commented 1 year ago

Please see astronomical formula collections. 5 or 2.5 are not just our arbitrary constants, they are part of the standardized formula. You can edit the actual data items, slope_parameter and absolute_magnitude in ssystem_minor.ini to tweak comet magnitudes if you find better values for absolute_magnitude and slope_parameter.

axd1967 commented 1 year ago

Please see astronomical formula collections. 5 or 2.5 are not just our arbitrary constants, they are part of the standardized formula.

sigh.

as I write, 5 in 5 log (d) in the formula is a generally accepted constant, I never wrote that those are "your" arbitrary constants. (I did not encounter 2.5 yet).

gzotti commented 1 year ago

You can encounter 2.5 on one of the pages given as source, http://www.ayton.id.au/gary/Science/Astronomy/Ast_comets.htm#Comet%20facts. When I encountered this magnitude implementation (that predates my activities here), I compared with what Meeus wrote and concluded that Meeus' κ=2.5*slope_parameter. With the information from this website we can finally conclude that the value should actually be called photometric_parameter. However, these are nitpicks that don't change the result.

axd1967 commented 1 year ago

https://github.com/Stellarium/stellarium/blob/21cfcec5705ea1c857e7cbdc7363345b7d5cd343/src/core/modules/Comet.cpp#L240

Agreed - nitpicking, yet it clarifies the code.

The second advantage of using a constant is that a developer can search the entire sourcecode for the presence of this concept, which is far more difficult (if not bugged) when having to search for "5".

Developers should be encouraged to minimize the use of literals, in favour of constants, preferably defined once only (the inconvenience being a risk of introducing too much dependencies between modules). This will also help refactoring, by thinking through which functions use those constants and are likely to be concentrated in one place.

All this is "butter and bread" for software engineers.

All this will be beneficial to software maintenance.

gzotti commented 1 year ago

Do you propose to define constants in a²+b²=c²?

10110111 commented 1 year ago

As a very minimum, all literal data (2.5, 5, ...) should be replaced by constants

Taking this blindly is following a cargo cult. If the formula is an empirical model where all parameters are fixed by a fit (especially "standardized"), and they don't have any particular physical meaning, there's no sense in naming these constants. Doing this would make reading harder while adding no benefit at all.

It might make sense to name the constants if you define an additional quantity with the same parameters, but sometimes even this may be better to simply document in a comment.

axd1967 commented 1 year ago

(There are more arguments than to purely name the constants for "cargo cult" reasons - pls re-read https://github.com/Stellarium/stellarium/issues/2932#issuecomment-1365942699)

axd1967 commented 1 year ago

Do you propose to define constants in a²+b²=c²?

There we go again - making me look ridiculous. And then I'm labeled as toxic. Once more, it's not me who started this! Honestly, I think this is just a form of trolling.

For those who might not be aware, I've been doing software engineering for over 15 years (and been dealing with software for almost my entire life). I've been participating (as developer) in a multi radar software project consisting of millions of lines of C, C++, Perl, Bash and others, all mixed together, for a major European corporation. I've worked with virtual machines, I've been migrating projects from ClearCase to SVN and Git. So please stop making fun of me.

And oh yes, I've contributed to several aspects of Stellarium - by contributing with code, by helping users, by helping in translations, by introducing small changes in the way Git is used here. Even financially I've contributed a bit.

@xalioth

(ps - see also https://en.wikipedia.org/wiki/Magic_number_(programming))

alex-w commented 1 year ago

@axd1967 sometimes you show a bad (or at least mediocre) knowledge of fundamental theoretical basins in astronomy and physics (sometimes in computer science too) - this fact in connection with your manner in the writing some sentences and statements may be reading like at least as very strange proposals.

10110111 commented 1 year ago

pls re-read #2932

OK, from there:

The second advantage of using a constant is that a developer can search the entire sourcecode for the presence of this concept, which is far more difficult (if not bugged) when having to search for "5".

If a reader has to search for a concept, they'd better know what it's called. And if the constant has no widely-used name, tough luck. It may be named anything in other parts of the code. Even more so: if there are reimplementations of this in other parts of the code, the developers already didn't know of the present one (or else they'd reuse it), so why would you expect to find anything structurally similar? Better grep for references to papers, using keywords such as author's name or parts of the paper title, or look for generic phrases or words like "comet", "magnitude" near each other, etc.

Don't think that I'm opposed to naming constants in general — no. But one has to use common sense and weigh merits of this vs code readability in each particular case, not just blindly follow the "never use goto", "always name literals" or similar dogmas. Only sith deal in absolutes.

alex-w commented 1 year ago

@10110111 important addition for your explanation - the value of "constant" will be different for different unit of measurements and we use this fact!

gzotti commented 1 year ago

You can also read the program manual for user-targeted information. Appendix, equation D.5.

axd1967 commented 1 year ago

@axd1967 sometimes you show a bad (or at least mediocre) knowledge of fundamental theoretical basins in astronomy and physics (sometimes in computer science too) - this fact in connection with your manner in the writing some sentences and statements may be reading like at least as very strange proposals.

And every time the word "you" creeps in, a conversation risks to degenerate into name calling and personal attacks. That's where the conversation ends.

@xalioth