Skip to content

Fix for export w/ UTC timestamps#169

Merged
StokesMIDE merged 1 commit intodevelopfrom
feature/ES-871_utc_timestamps
Nov 19, 2025
Merged

Fix for export w/ UTC timestamps#169
StokesMIDE merged 1 commit intodevelopfrom
feature/ES-871_utc_timestamps

Conversation

@StokesMIDE
Copy link
Member

Fixes time scaling issue when exporting with UTC timestamps. Also updated the annotations and docstrings of the export functions/methods edited, plus some cleanup.

@codecov-commenter
Copy link

Codecov Report

❌ Patch coverage is 71.42857% with 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 75.88%. Comparing base (f08c465) to head (09ef446).

Files with missing lines Patch % Lines
idelib/matfile.py 70.58% 5 Missing ⚠️
idelib/dataset.py 75.00% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop     #169      +/-   ##
===========================================
+ Coverage    75.84%   75.88%   +0.03%     
===========================================
  Files           15       15              
  Lines         4450     4453       +3     
===========================================
+ Hits          3375     3379       +4     
+ Misses        1075     1074       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Member

@pscheidler pscheidler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm sure you've tested this so it will help the Iocene folks, so I don't want to get in the way, but a few things seem odd to me:

  • Are you comfortable with timeScalar and how timestamps are stored? Instead of adding in rowTimeScalar could you just use timeScalar? Or have the caller use it?
  • It seems like now, if we store using the UTC time we convert timestamps to microseconds, but I guess we store in seconds if it is not UTC? It seems like a footgun
  • Test coverage for this looks light, any thoughts?

This is eldrich code, and I'm sure we'll all be happier when it is buried. Near term, I don't think we need anything more than documentation and maybe jira tickets.

rowTimeScalar = 1e-06
timeScalar = 1
else:
rowTimeScalar = 1
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't rowTimeScalar still be 1e-6? Our timestamps are all still in microseconds, right? We just want to start the time from 0

@StokesMIDE
Copy link
Member Author

@pscheidler

* Are you comfortable with timeScalar and how timestamps are stored? Instead of adding in `rowTimeScalar` could you just use `timeScalar`? Or have the caller use it?

The ways EventArray.exportCsv() and exportMat() are implemented differ; the latter keeps track of a time scalar in a different location. I'd really rather remove the time scalar from the parameters, but I don't want to change the API without a more major release.

* It seems like now, if we store using the UTC time we convert timestamps to microseconds, but I guess we store in seconds if it is not UTC? It seems like a footgun

Other way around... the timestamps are typically microseconds; epoch time is seconds, so the timestamps are scaled to seconds. It seemed weirder to use a nonstandard epoch and units.

* Test coverage for this looks light, any thoughts?

It is. The tests are kind of a mess, simultaneously over-engineered and brittle. That much isn't entirely my fault, at least.

This is eldrich code, and I'm sure we'll all be happier when it is buried. Near term, I don't think we need anything more than documentation and maybe jira tickets.

I've noted elsewhere that the export code is kind of crap. The CSV export is ancient, and the MAT code was meant to be a temporary hack.

@StokesMIDE StokesMIDE merged commit 09ef446 into develop Nov 19, 2025
37 of 55 checks passed
@StokesMIDE StokesMIDE deleted the feature/ES-871_utc_timestamps branch November 19, 2025 13:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants