-
Notifications
You must be signed in to change notification settings - Fork 2
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Feature: Upload external datasets #112
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good - a few non-blocking notes below, but I'm going to approve and merge this for today's release. I've tested locally with the examples and all seems to be working correctly. We still need to document this feature and potentially add some better tests & coverage for some edge cases/handling for other data types, but this is a first pass at an experimental feature that users are asking for, so I think it's worth releasing v1 as is. Thanks @duranb !
// Create a lookup for the profile name's index in each CSV row | ||
const headerIndexMap: Record<string, number> = parsedCSV[0].reduce( | ||
(prevHeaderIndexMap: Record<string, number>, header: string, headerIndex: number) => { | ||
if (new RegExp(timeColumnKey).test(header)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One thing to note here is that this will loosely match any header that contains the substring time_utc
, eg. event_time_utc
will match. I think this is OK here since there shouldn't be any ambiguity, since there should only be one timestamp column & the others should be numeric, but worth calling out & considering if it should be a strict equality match
expect(getTimeDifference('2024-245T00:01:00.0', '2024-245T12:02:00.0', 6)).toEqual(43260000000); | ||
expect(getTimeDifference('2024-243T00:01:00.0', '2024-245T12:02:00.0', 6)).toEqual(216060000000); | ||
}); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could use some more unit tests here for better coverage of the parsing functions in time.ts
Partially resolves NASA-AMMOS/aerie-ui#1424
Refer to UI PR for testing procedure.