search

SuperDARN / rst

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

Map_addimf and map_addmodel issue with missing imf data #201

Closed ksterne closed 5 years ago

ksterne commented 5 years ago

@egthomas and I found that if the IMF data is missing while loading in data during the map_addimf process, the current code is using a missing value default of 9999 for all three IMF values. This needs to be corrected and likely restore the functionality presented in the documentation of using the last valid IMF data.

In addition, map_addmodel isn't recognizing the very large values and correcting for these.

@egthomas and I found these while looking at data from 20161230.2320 and later that day where recently (in the last week or so) downloaded OMNI data doesn't have any valid IMF data. So, this issue is a to-do reminder for @egthomas and I to correct these issues. I'll take the map_addimf code and he'll take on the map_addmodel code...eventually.

ksterne commented 5 years ago

So a bit of refinement here from what I think I'm seeing on the IMF data loaded in using the -if flag, it seems as though the problem occurs when there is missing data at the end of a file (in my case, I've split the IMF data up into daily chunks). I think the way the code is now, that is the way it should happen, so I'll look at why things are happening that way.

However, if the IMF data is missing somewhere in the middle of the file, the code interpolates between the two good values. For instance, the current OMNI data has a large chunk of data missing between 20161230.2100 and 20161230.2150. The Bx is 2.02 at 20:59 and 3.10 at 21:51 for this day. The file generated after map_addimf has a IMF.Bx value that starts at 2.02 and slowly goes up to 3.10 during the gap in IMF data. So I'll update the documentation for this the documentation to capture that.

I haven't yet looked at the other extreme case of the data at the beginning of a file not being valid. I'll try to see if I can easily find a day that has this characteristic to check as well.

egthomas commented 5 years ago

@ksterne so does map_addimf only insert the 9999 missing values if there is not an earlier/later record in the text file to interpolate towards?

I've thought a bit more about the map_addmodel behavior and I think we need to be careful about choosing which "default" pattern to use when no solar wind information is available. Choosing the largest magnitude model bin by multiplying the 9999 missing values together probably isn't the right way to do it, but then again selecting the smallest model bin probably isn't either. We do want to leave those 9999's in the map file to indicate the solar wind information was missing.

ksterne commented 5 years ago

@egthomas, yes to the 9999 missing value being used if there's not another record (at least if there isn't one later...again need to check starting issue, but might be able to fudge that by removing some records) to interpolate towards.

The issue seems to come up in the timing check in the map_addmodel.c file:

https://github.com/SuperDARN/rst/blob/556e010a84d955e9105ab7a61cfeed46f677ac2a/codebase/superdarn/src.bin/tk/tool/map_addimf.1.18/map_addimf.c#L329-L330

Here stime and etime are epoch times that represent some kind of start and end time relating to the IMF data that's read in. The tme variable is passed in to the findvalue() function as the start time of the period read from the input "map" file. Since records keep going in the "map" file and there isn't a corresponding time in the IMF data, the condition of tme > etime is met and the function returns. I tested this out by commenting these out and noticed things kept going...and I think it kept the last good IMF value....not sure on that.

For now, I'm wondering why are these lines needed? Maybe @sshepherd, or @pasha-ponomarenko might have some knowledge on these lines? I could see that they could be useful in the case where something in: https://github.com/SuperDARN/rst/blob/556e010a84d955e9105ab7a61cfeed46f677ac2a/codebase/superdarn/src.bin/tk/tool/map_addimf.1.18/map_addimf.c#L317-L328 goes wrong. Maybe the conditions for lines 329 and 330 above just need to be expanded to include cases where i and/or cnt equal sinx and einx or maybe when sinx and einx are the same. This is where I'm at, but wanted to post here to see if there was any wisdom on what these lines might be checking for other than an error in the data or code somewhere.

Based on the logic here, I'm 95% certain this issue will happen if there is missing data at the beginning of an IMF file too as it won't have a good place to start...but I need to test this.

egthomas commented 5 years ago

@ksterne at some point between RST 2.11 and 3.1 Rob added the interpolation scheme which probably caused the discrepancy with the documentation:

https://github.com/SuperDARN/rst-archive/blob/rst.3.1/codebase/superdarn/src.bin/tk/tool/map_addimf.1.17/.rst/log#L35

So even @sshepherd who did most of the updates to map_addimf may not know the original intent of those particular lines in the findvalue function. If it's bugging out then I would just go ahead and fix it.

mts299 commented 5 years ago

Can we close this issue since there is PR #204 for it?

ksterne commented 5 years ago

@mts299, I don't think we can close this one for now as #204 hasn't been merge (hint hint) and as well that pull request only solves one of 2 issues here. @egthomas, did you have more thoughts on what might be done for map_addmodel to protect against bad values? I'm a little fuzzy on what you were thinking map_addmodel needed here. Does it still need some modifications or are the edits in #204 filtering out bad values for map_addmodel?

mts299 commented 5 years ago

@ksterne personally I like to close issues if there are PR's for it so that the discussion can move there but it is your issue :)

ksterne commented 5 years ago

@mts299, you'll follow in #212 that this was a 2 parter and only half had been addressed. My other preference is to wait until the PR's been merged in so that I can think of closing as a problem/issue solved. There's a happy middle somewhere though.

mts299 commented 5 years ago

@ksterne Sorry got confused on the two-parter points here and what IMF bad values meant. I thought those were separate issues.

I always see PR's as the solution to an Issue so once a PR was created (In other words the issue was solved) then the issue could be closed, and the conversation could move into the PR for better workflow.

Maybe leave it up to the owner of the issue to close it (if it is not outdated) and someone else to merge the PR? ¯_(ツ)_/¯ Personal preferences, gotta love them.