-
Notifications
You must be signed in to change notification settings - Fork 56
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
DSP Improvements #324
DSP Improvements #324
Conversation
Codecov Report
@@ Coverage Diff @@
## main #324 +/- ##
==========================================
- Coverage 33.11% 33.11% -0.01%
==========================================
Files 73 73
Lines 6184 6185 +1
==========================================
Hits 2048 2048
- Misses 4136 4137 +1
Continue to review full report at Codecov.
|
a_out[0] = w_in[int(t_in)] | ||
i = int(t_in) | ||
w = t_in-i | ||
a_out[0] = (1-2)*w_in[i] + w*w_in[i+1] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is (1-2)
doing there? Should it be (1-w)
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also, I think it would be clearer to rename t_in
as i_in
since it is an index, not a time...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
D'oh, fixed!
src/pygama/lgdo/lh5_store.py
Outdated
@@ -622,8 +622,9 @@ def write_object(self, | |||
|
|||
# scalars | |||
elif isinstance(obj, Scalar): | |||
if name in group: | |||
log.debug(f"overwriting '{name}' in '{group}'") | |||
if wo_mode == 'o' and name in group: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also feel like if name
is in group
and wo_mode
is 'w'
then we should emit an error here. @iguinn do you agree?
Also wo_mode = 'a' should be okay for overwriting scalars, according to the
write_object` docstring.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made this change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, sounds good. I guess the other case is 'of' but in that case I think no check is needed since the file's overwritten so it shouldn't be possible to run into this? Either way, raising an error ourselves is a good idea, as before it was raising a somewhat confusing h5py error.
@iguinn, I don't like the idea of changing the behaviour of the processor and like to have it as it is now. I would rather implement a specific processor that supports different types of interpolation and can replace fixed_time_pickoff if the user wants so |
As of Ian’s PR, the code does not change behavior based on the cast of an input parameter, it does an interpolation every time. To my mind, this is the desired behavior of this processor. It gives consistent output independent of pre-summing, for example. |
Ok, I"ve reverted the changes to fixed_time_pickoff. I'll create an issue for it, but until we fix it we shouldn't start using presumming |
I can't tell either, the error message is pretty unhelpful here...It looks to me like other pull requests are running into the same issue, though, so it's not clear that changes here are the cause. If this PR is actually the cause, I did make changes to a function signature (specifically an annotation) on line 26 of build_dsp.py, so it's possible that could have caused it? |
The build_documentation failure is a bug in sphinx. I'm going to turn off this requirement for merging and merge. |
chan#/raw
; now it will work with justchan#