Fix bug in VSOClient where ill formatted times would cause a crash - #8702
Conversation
|
|
||
| def test_QueryResponse_build_table_bad_time_format(mocker): | ||
| mocker.patch("sunpy.net.vso.vso.build_client", return_value=True) | ||
| records = (MockQRRecord(start_time="20220102030462"),) |
There was a problem hiding this comment.
I suggest testing using a more wildly incorrect time, such as the 73rd month of the year. At some point, we could conceivably modify parse_time() to handle slight overages in seconds (similar to how we catch and fix "24:00:00").
There was a problem hiding this comment.
How about?
| records = (MockQRRecord(start_time="20220102030462"),) | |
| records = (MockQRRecord(start_time="20169998979695"),) |
Yea I guess if we made some assumptions we could do that do something for these cases.
But I had a look at the underling fits file in one case where the time in the json response is '20160518065962' and the time related headers from the file kanz_caiik_fi_20160518_065959.fts are
DATE_OBS= '2016-05-18T06:59:59Z' / Date of observation
DATE-OBS= '2016-05-18T06:59:59' / Date of observation
...
EXP_TIME= 2.333 / Exposure Time [ms]
So not clear to me where the 62 for this specific files come from unless they round up and take ms as s to get 59+3?
I've no idea if a 2.33ms exposure time makes sense for a KANZELHOEHE CIIk filter image.
There was a problem hiding this comment.
So not clear to me where the 62 for this specific files come from unless they round up and take ms as s to get
59+3?
Ha ha, it certainly gives the appearance of someone mistaking milliseconds for seconds. Google searching indicates that the exposure time can be as long 35 ms, so it'd be good to try to find a long exposure to see if the end time is pushed out by that many seconds. Otherwise, it could simply be that 3 seconds is a canonical observation interval (e.g., maybe how long it takes to read out the sensor).
There was a problem hiding this comment.
@ayshih @samaloney does this thread need resolving before we merge this?
There was a problem hiding this comment.
Yea I think the 99th month the the 98th day ... 97, 96, 95, are wildly enough wrong 😆
|
Don't think the test fails are related |
3050de2 to
f98878a
Compare
* Now unparable times are replaced with a dummy value and the entry is masked, similar to how missing value were already treated.
f98878a to
6ff909b
Compare
Co-authored-by: Stuart Mumford <stuart@cadair.com>
This comment was marked as duplicate.
This comment was marked as duplicate.
|
@meeseeksdev backport to 8.0 |
…2-on-8.0 Backport PR #8702 on branch 8.0 (Fix bug in VSOClient where ill formatted times would cause a crash)
5 participants
PR Description
Closes #8687 by treating dates which can't be parsed in a similar way to missing dates, use a dummy value and mask the corresponding entry in the table.
AI Assistance Disclosure
AI tools were used for: