Skip to content
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

Writer timestamps handling improvements #492

Merged
merged 10 commits into from
Nov 4, 2022

Conversation

dbkeitel
Copy link
Collaborator

@dbkeitel dbkeitel commented Nov 2, 2022

  • check for missing detectors if timestamps are a string
  • allow single-column timestamps files (and warn for Ncol>1)

@dbkeitel dbkeitel added enhancement New feature or request data issue with data handling/writing labels Nov 2, 2022
Copy link
Collaborator

@Rodrigo-Tenorio Rodrigo-Tenorio left a comment

Choose a reason for hiding this comment

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

I agree with the approach so far. Final changes dependent upon opened discussion.

pyfstat/make_sfts.py Show resolved Hide resolved
Co-authored-by: Rodrigo Tenorio <rodrigo.tenorio@ligo.org>
@dbkeitel
Copy link
Collaborator Author

dbkeitel commented Nov 2, 2022

We may want to support genfromtxt_kwargs if you think it would be usable.

Could, but I'd leave that as a future enhancement request? Maybe together with having a central timestamps parsing util that DetectorStates can share?

@dbkeitel
Copy link
Collaborator Author

dbkeitel commented Nov 2, 2022

Could, but I'd leave that as a future enhancement request?

-> #493

@dbkeitel
Copy link
Collaborator Author

dbkeitel commented Nov 2, 2022

Left to do: sanity check that the first column is actually int

@codecov
Copy link

codecov bot commented Nov 2, 2022

Codecov Report

Merging #492 (545bf4e) into master (c502303) will increase coverage by 0.18%.
The diff coverage is 100.00%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #492      +/-   ##
==========================================
+ Coverage   73.21%   73.39%   +0.18%     
==========================================
  Files          21       21              
  Lines        4304     4311       +7     
==========================================
+ Hits         3151     3164      +13     
+ Misses       1153     1147       -6     
Flag Coverage Δ
unittests 73.39% <100.00%> (+0.18%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
pyfstat/make_sfts.py 88.19% <100.00%> (+1.27%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c502303...545bf4e. Read the comment docs.

Rodrigo-Tenorio
Rodrigo-Tenorio previously approved these changes Nov 3, 2022
Copy link
Collaborator

@Rodrigo-Tenorio Rodrigo-Tenorio left a comment

Choose a reason for hiding this comment

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

Added check for integer timestamps and test case for different warnings.

tests/test_make_sfts.py Outdated Show resolved Hide resolved
tests/test_make_sfts.py Outdated Show resolved Hide resolved
tests/test_make_sfts.py Show resolved Hide resolved
tests/test_make_sfts.py Show resolved Hide resolved
pyfstat/make_sfts.py Outdated Show resolved Hide resolved
@dbkeitel dbkeitel merged commit b254b80 into master Nov 4, 2022
@dbkeitel dbkeitel deleted the allow-single-column-timestamps branch November 4, 2022 14:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
data issue with data handling/writing enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants