search

SuperDARN / rst

Radar Software Toolkit (RST)
https://superdarn.github.io/rst/
GNU General Public License v3.0
22 stars 18 forks source link

Constants values #238

Closed mts299 closed 4 years ago

mts299 commented 5 years ago

Going through the code I notice we tend to use different constants values with varying precision. This doesn't seem like a good practice from a developer point of view or scientifically. This may have happened by not having developers guide but wondering if we should look into fixing the current inconsistencies, then document?

For example: When I was working on #150 I noticed we were using two different speed of light values in FITACF 2.5 https://github.com/SuperDARN/rst/blob/60c16a047f77bf8e73ceb3d32448680d4a8d813d/codebase/base/src.lib/math/rmath.1.8/include/rmath.h#L44 and 3.0 https://github.com/SuperDARN/rst/blob/a3858d423ef370cda686471bada1d95f5319040b/codebase/superdarn/src.lib/tk/fitacf_v3.0/src/determinations.h#L39

I understand we want to keep the two for reproducibility; however, this became a challenge when shifting the two fitting algorithms to libraries with the elevation calculations to libraries. I think it may have come up in @aburrell results when testing the PR. Which now realizing this could be considered a bug or a very unknown caveat.

I have also come across several definitions of PI. https://github.com/SuperDARN/rst/blob/444abd4f38ff18bb6f9a25b6272254fe8af8b4d0/codebase/superdarn/src.lib/tk/sim_data.1.0/include/sim_data.h#L8 https://github.com/SuperDARN/rst/blob/444abd4f38ff18bb6f9a25b6272254fe8af8b4d0/codebase/analysis/src.lib/astalg/astalg.2.0/include/astalg.h#L65-L67 Granted this is assuming PI is not already set so it sort of handles the problem: https://github.com/SuperDARN/rst/blob/444abd4f38ff18bb6f9a25b6272254fe8af8b4d0/codebase/analysis/src.lib/aacgm/aacgm.1.15/src/math.h#L35

I was thinking a solution would be to correct the constant inconsistencies and use one file or library (like math.h) to contain the ones developers should use. In the Release notes for the release that some values will change between RST versions because of this change.

This will also avoid some typo bugs or round off values.

ksterne commented 4 years ago

Lets try to get this going very soon here as there's 2 other issues that are depending on it (#257 and #256). I'm guessing there are 2 approaches to this:

  1. Define the constants ourselves, but use a common value for all (in one library or some such)
  2. Use an external library for constant values.

It seems as though we're ruling out the 2nd option which I think is good. The external library would create a dependency and it might change (or be non-existent) between platforms.

Is a quick and dirty version of this finding all the places where the speed of light and pi are defined and just setting them to the same value? Are there more constant values that are being used?

There's the issue of what precision we should use, but lets come back to that after this.

mtwalach commented 4 years ago

I agree that approach 2 is more future proof and (maybe?) user friendly.

I think another constant that comes up is the Earth radius.

ecbland commented 4 years ago

@mtwalach could you please clarify...

I agree that approach 2 is more future proof and (maybe?) user friendly.

This seems to be the opposite view expressed by @ksterne

It seems as though we're ruling out the 2nd option which I think is good. The external library would create a dependency and it might change (or be non-existent) between platforms.

mtwalach commented 4 years ago

Sorry - I got my numbers switched and actually meant 1!

ksterne commented 4 years ago

Is this good to close now that #281 has been merged?

ksterne commented 4 years ago

@mts299, feel free to re-open this, but Emma and I seemed to think this is done for now and what hasn't been accomplished as been captured in the 'Physical and mathematical constants' project. So, I'm closing this issue.