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

Tabor segment deduplication #797

Merged
merged 12 commits into from
Oct 5, 2023

Conversation

terrorfisch
Copy link
Member

@terrorfisch terrorfisch commented Sep 28, 2023

Should fix #795

  • test with simulator

@github-actions
Copy link

github-actions bot commented Sep 28, 2023

Test Results

       6 files         6 suites   5m 20s ⏱️
1 219 tests 1 163 ✔️   56 💤 0
7 280 runs  6 850 ✔️ 430 💤 0

Results for commit c8ef798.

♻️ This comment has been updated with latest results.

@terrorfisch terrorfisch marked this pull request as ready for review September 28, 2023 12:24
@terrorfisch
Copy link
Member Author

@Nomos11 Can you test if this works?

@Nomos11
Copy link
Collaborator

Nomos11 commented Sep 28, 2023

so far it seems to give me an IndexError at to_upload_size = np.sum(new_segment_lengths[unknown] + 16), which implies that it still tries to find places for all redundant waveforms, I'll investigate that further

@Nomos11
Copy link
Collaborator

Nomos11 commented Sep 28, 2023

It seems like the segment_lengths need to be adapted to throw out duplicates, which should be fixable relatively easily.
E.g., something like

unique_segments = list(segments.keys())
return (unique_segments, [segment.num_points for segment in unique_segments]), waveform_to_segment

at the end of _calc_sampled_segments or some more efficient variation

@terrorfisch
Copy link
Member Author

Does it work now?

@Nomos11
Copy link
Collaborator

Nomos11 commented Sep 29, 2023

From looking at the code I am a bit confused - does cutting off the array choose the correct values? the segment_lengths can contain subsequent duplicate values, which are not thrown out here, giving the incorrect values to my understanding

@terrorfisch
Copy link
Member Author

You are right. I did not think it through.

@terrorfisch
Copy link
Member Author

@Nomos11 Should work now

@terrorfisch terrorfisch merged commit 8aadcd6 into master Oct 5, 2023
9 checks passed
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.

Tabor: redundant waveform upload
2 participants