search

SuperDARN / rst

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

Integration of cmpfit branch point causing small differences in fitex files versus VTRST3.5 #30

Closed ksterne closed 6 years ago

ksterne commented 7 years ago

As found in #16, somewhere between the VTRST3.5 tag and that pull request, a change in the codebase appeared to be causing a differences in the p_l and w_l variables in the result of make_fitex2 -new. Only looking at a few of the first differences, it appears as though the change in the values is very small, but every difference, even within a 2 hour file, was not categorized. Even if all of the differences are small, this make may comparing datasets tricky if the contents of the files are different.

pasha-ponomarenko commented 7 years ago

I looked at the code (fitacfex2.c, lines 415-438), and it looks like estimating p_l and w_l does not involve MPFIT. Instead, it is based on a least-square procedure replicated from the Numerical Recipes, nrfit.c.

egthomas commented 7 years ago

To test this issue, I followed these steps on a Debian 8.7.1 virtual machine:

  1. clone develop branch of VTRST3.5 repository from vtsuperdarn account
  2. compile VTRST3.5 (painfully)
  3. execute make_fitex2 -new 20161012.1601.00.bks.rawacf > 20161012.1601.00.bks.fitex.vtrst
  4. clone develop branch of rst repository from superdarn account
  5. compile RST
  6. execute make_fitex2 -new 20161012.1601.00.bks.rawacf > 20161012.1601.00.bks.fitex.rst
  7. execute dmapdump -d 20161012.1601.00.bks.fitex.vtrst > 20161012.1601.00.bks.dump.vtrst
  8. execute dmapdump -d 20161012.1601.00.bks.fitex.rst > 20161012.1601.00.bks.dump.rst
  9. execute diff 20161012.1601.00.bks.dump.vtrst 20161012.1601.00.bks.dump.rst | tee output_file.txt

Examining the contents of output_file.txt, the only differences between the VTRST3.5 output and RST output are the string "origin.time" values. This is the same result I observed when comparing the output of make_fitex2 of the develop (prior to merging) and cmpfit_cleanup branches of this repository.

So in summary, I am unable to replicate this issue on either an Ubuntu or Debian system.

ksterne commented 7 years ago

@egthomas, that is certainly interesting....I'll have to take a look at some more testing to see what is going on here. As @pasha-ponomarenko noted, it could be that it's a different version of cmpfit, or did you use the version that is with the VTRST3.5? I think I remember we had ended up including that package within that repo...but maybe I'd left it out for copyright issues.

egthomas commented 7 years ago

The above test used the version of cmpfit (1.2) which is contained in both the VTRST3.5 and RST repositories. We haven't integrated the newer version of cmpfit (1.3a) that Pasha identified into any of the RST repositories yet.

pasha-ponomarenko commented 7 years ago

After a closer look I identified that in fitacfex2.c no routines using mpfit are utilised. While the lmfit.h is called in the header, the only procedures used from that package are following: calc_phi0 setup_fblk lm_noise_stat calc_err All of them are AJ's replicas of the respective FITACF routines, and neither of them uses the mpfit library. Therefore, the source of the discrepancy observed by Kevin should be somewhere else.

egthomas commented 7 years ago

Is this still a problem @ksterne ? Have you had a chance to do any more testing?

ksterne commented 7 years ago

I have not had anytime to do more testing. I would say this is still an issue.

egthomas commented 6 years ago

So, it's been 6 months - any movement on this issue? From looking at the respective histories of the make_fitex2 binary and fitacfex2 library I still don't see how any changes to the RST would impact the values in fitex2 files.

ksterne commented 6 years ago

Hey @egthomas, I haven't been able to come back to this issue here. I certain want to since you noted that you didn't see the problem. Maybe it'll get on my to-do list next week...

egthomas commented 6 years ago

So it's been a full year and more than 500 commits later with no evidence of this issue. At the time I compared VTRST3.5 and pre-RST4.0 output on two different Linux distributions and found no differences, and @pasha-ponomarenko confirmed that make_fitex2 and fitacfex2 do not depend on the cmpfit library. I move that we close this issue.

ksterne commented 6 years ago

I'll sound like a broken record and I just haven't gotten to testing this yet. I'd still like to investigate this though. I was definitely seeing differences in fit-level data between the two repos. It could be something local to VT though.

aburrell commented 6 years ago

Fair enough. We should put a log warning when the routine is run, so people who may use it know that there are potential issues.

On 9 Mar 2018, at 09:27, Kevin Sterne notifications@github.com wrote:

I'll sound like a broken record and I just haven't gotten to testing this yet. I'd still like to investigate this though. I was definitely seeing differences in fit-level data between the two repos. It could be something local to VT though.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/SuperDARN/rst/issues/30#issuecomment-371844481, or mute the thread https://github.com/notifications/unsubscribe-auth/AGuC_lYjpjXEg64GrJVITAZ_lVWMe1Unks5tcp9HgaJpZM4MVCFu.

ksterne commented 6 years ago

Eh, I'm not sure a warning message is warranted yet. I'm ok with keeping this as an issue until I can revisit things and see where I'm at.

aburrell commented 6 years ago

Ok, you’ve looked at the routine the most so it’s your call.

On 9 Mar 2018, at 10:02, Kevin Sterne notifications@github.com wrote:

Eh, I'm not sure a warning message is warranted yet. I'm ok with keeping this as an issue until I can revisit things and see where I'm at.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/SuperDARN/rst/issues/30#issuecomment-371855476, or mute the thread https://github.com/notifications/unsubscribe-auth/AGuC_m2q-5NPqtRD5o8R3qN4hpoX8PDoks5tcqeugaJpZM4MVCFu.

sshepherd commented 6 years ago

I think that since this issue has sat idle for over a year, it should be closed. Kevin can do an internal check if and when he has time, but keeping an issue open for a year without anyone looking at it just seems a bit absurd.

On Fri, Mar 9, 2018 at 11:10 AM, Angeline Burrell notifications@github.com wrote:

Ok, you’ve looked at the routine the most so it’s your call.

On 9 Mar 2018, at 10:02, Kevin Sterne notifications@github.com wrote:

Eh, I'm not sure a warning message is warranted yet. I'm ok with keeping this as an issue until I can revisit things and see where I'm at.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub < https://github.com/SuperDARN/rst/issues/30#issuecomment-371855476>, or mute the thread https://github.com/notifications/unsubscribe- auth/AGuC_m2q-5NPqtRD5o8R3qN4hpoX8PDoks5tcqeugaJpZM4MVCFu.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/SuperDARN/rst/issues/30#issuecomment-371857657, or mute the thread https://github.com/notifications/unsubscribe-auth/AC2DPwCEgwFqNrU0x2Oh-znYAxILmuVhks5tcqlbgaJpZM4MVCFu .

egthomas commented 6 years ago

I think we agreed during the 22 March 2018 telecon this issue would be closed if no action was taken by the time of the SD workshop, so closing.

ksterne commented 6 years ago

Yep, absolutely agree. It doesn't seem to be a big issue, so I guess we can keep in in the closed part for future reference.