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

fixes #256 - checking input to env with filters #263

Merged
merged 3 commits into from
Feb 11, 2024

Conversation

tremblap
Copy link
Member

check the input to env and envgate and stores the last valid value to replace non-finite values with

@AlexHarker
Copy link
Contributor

See my last comment on the issue. I think there's a better solution.

Copy link
Member

@weefuzzy weefuzzy left a comment

Choose a reason for hiding this comment

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

Logically looks fine. I have only stylistic consistency nitpicks, at least one of which should have been caught by clang-format:

  • We've been using newline before braces everywhere, IIRC. clang-format should be doing this.
  • Prefer brace-initialization of the member variable if that's how all the others are done
  • Maybe this has just passed the threshold of fiddly-ness where the code duplication between Envelope.hpp and EnvelopeGate.hpp is tiresome. Could think about pulling out the whole test if finite and filter-twice-if-needed saga into its own thing

@tremblap
Copy link
Member Author

I have satisfied @weefuzzy first 2 demands. clang-format revealed other discrepancies which I've pushed as we may as well clean it here. As much as I agree with the spirit of the 3rd request, I'd do a proper refactor of these 2 things to make them more efficient in block process (they are doing sample per sample now which is suboptimal)

@weefuzzy
Copy link
Member

(they are doing sample per sample now which is suboptimal)

Don't think there's a way around that for these algorithms because they need per-sample feedback.

But, in any case, is was the formatting consistency that was the thing here. I'd be inclined to go with this unless there's a clear functional reason to play with the other solution ahead of just fixing this bug. Feels to me like a we-can-revisit-if-it's-a-problem kind of thing.

@tremblap
Copy link
Member Author

ok thanks both. Let's put that fix in and consider other options as future improvements

@tremblap tremblap merged commit 3335b40 into main Feb 11, 2024
3 checks passed
@tremblap tremblap deleted the fix-nan-blowing-envslicers branch February 11, 2024 17:31
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