Skip to content

Fix offset when using synthetic noise with BioCAM's sparse compression #1746

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

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

Conversation

b-grimaud
Copy link
Contributor

Simple fix for #1743

Copy link
Contributor

@zm711 zm711 left a comment

Choose a reason for hiding this comment

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

Thanks @b-grimaud. I think this makes sense since we need the offset. We are planning a release in the next couple days, so we will need to discuss this before merge. It may or may not make it in this release, but either way I think it would be more helpful if you put a comment in explaining why we need the offset of 2048 and you can even put in your github issue. something like

# offset of 2048 necessary b/c ....
corrected_data = data + 2048
return corrected_data

@zm711 zm711 added this to the 0.15.0 milestone Jul 7, 2025
@zm711
Copy link
Contributor

zm711 commented Jul 8, 2025

We will have @alejoe91 doublecheck this math since he worked with you on the last set of PRs. So you can wait to add the comment until he says its the right idea or just add the comment now before he reviews.

@b-grimaud
Copy link
Contributor Author

I added some context, hope it's clear enough !

@zm711
Copy link
Contributor

zm711 commented Jul 8, 2025

I added some context, hope it's clear enough !

That looks perfect to me. Like I said Alessio will have final say after review, but the logic makes sense to me.

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