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

Hk resample #19

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open

Hk resample #19

wants to merge 9 commits into from

Conversation

ahincks
Copy link
Contributor

@ahincks ahincks commented Jul 10, 2019

This adds a module for resampling data in a frame. Currently it is fairly skeletal, but just to show my basic idea for how to do this. In particular, comments welcome on:

  • The architecture (i.e., how the scheme is implemented).
  • Have I created the new frames correctly (e.g., the use of HKSessionHelper)?

@ahincks ahincks requested a review from mhasself July 10, 2019 01:15
@ahincks
Copy link
Contributor Author

ahincks commented Aug 19, 2020

Finally, a version with basic functionality! It works on the HK test_data on simons1.

[I haven't checked to ensure that I'm up-to-date on our python formatting guidelines, so please let me know if there is anything egregious that I should fix.]

Currently you can do the following:

  • Interpolate all data so that it has the same time-axis as a field chosen by the user (t_ref_field in HKResampler constructor).
  • The interpolation is basic: it doesn't try to do anything clever if there are large gaps in the data being interpolated.
  • There is not filtering for the case when you are downsampling, so there will be aliasing.

Bugs to fix:

  • Currently, if a provider is dropped at the appearance of a new status frame, all its data are just flushed immediately. However, if there is no recent data from the field providing the reference times, then not all the data can be processed in the provider that is dropped, and data will be missing. The correct solution is to delay writing the status frame (and frames from non-dropped providers) until dropped provider can write out all its data. But this requires more book-keeping. See comment labelled FIX1 in the code.
  • Possible bug: should all providers be flushed when a new session starts? Technically frames from the previous session could still appear in the stream. See comment labelled FIX2 in the code.

@ahincks
Copy link
Contributor Author

ahincks commented Aug 19, 2020

The coverage check seems to be failing. Does this have something to do with not enough code having tests?

@BrianJKoopman
Copy link
Member

The coverage check seems to be failing. Does this have something to do with not enough code having tests?

Yeah, it "fails" past a threshold. I bumped it to 3% when things were failing for any change at all. I do think some tests would be valuable to add. You mention it works on the test data on simons1. What was tested on each file set? Can those tests be added? Getting the data set into the test environment can be setup later.

@BrianJKoopman BrianJKoopman requested review from mhasself and removed request for mhasself August 26, 2020 18:52
@ahincks
Copy link
Contributor Author

ahincks commented Sep 8, 2020

Resampler basically works now in in the g3 module/pipeline framework, both for user-supplied timestamps and when a reference field is specified.

I think it can be merged if we want.

At the same time, there are still a few things to do/improve.

  • Integrate this into hk.getdata.
  • Make more efficient: currently takes ~40 seconds on my laptop to do the full set of HK test data. Can slow down a lot, though, if (for some reason) you want the output frames to be short.
    • Probably too much looping in python, plus the actual interpolation might need to be compiled.
  • Enable filtering for the scenario where you are downsampling to prevent aliasing.
  • Allow for different interpolation schemes.
  • Allow for options in how gaps in the data are treated.

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.

2 participants