SuperDARN / pydarn

Python library for visualizing SuperDARN Data
GNU Lesser General Public License v3.0
31 stars 11 forks source link

Tolerence of one second is too low when passing times to plot_fan #385

Closed billetd closed 1 month ago

billetd commented 2 months ago

BUG

364 introduced a bug(?) when making fan plots by passing a datetime to the scan_index keyword. As tolerance works by searching records, not scans, the default of dt.timedelta(seconds=1) causes maybe only one beam to be selected for plotting, or non at all depending on the time you pick. Setting tolerence = dt.timedelta(seconds=60) fixes this, but this might mess with the plotting if you use an actual index for scan_index.

What should the default be? Should we have a different keyword for selecting a time?

Priority

RemingtonRohel commented 2 months ago

I don't agree that this is a bug, this was the intended design of the function find_records_by_datetime(). I chose a small tolerance as default for my use case of wide-beam experiments, but it is still fully functional for narrow beam experiments as you mentioned.

The function actually matches any records within tolerance of your scan_index datetime value, so with tolerance = 60 seconds you will get two minutes worth of records (60 seconds on either side), centered around the scan_index time.

I agree though that it would be cleaner to have two separate kwargs, since passing a datetime as scan_index means that you haven't exactly passed in an index, you've passed in a time. Definitely could be much clearer. This would mean handling some kwargs more carefully, but that's not too difficult.

billetd commented 2 months ago

Yes I agree find_records_by_datetime() is working as intended, though maybe a default shouldn't be for wide-beam at this time as the majority of data is narrow beam, 1-min scans. Two keywords would be best but I don't think its a priority right now, at least not as much as changing the default tolerance.

With tolerance=60, I just realised it was because I was looking at twofsound data, so you need 60 seconds there because of alternating channels.

RemingtonRohel commented 2 months ago

Fair enough, I agree that changing the default is a good idea.