-
Notifications
You must be signed in to change notification settings - Fork 4
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
Filters "applied" booleans should be reversed #173
Labels
invalid
This doesn't seem right
Comments
kkappler
added a commit
that referenced
this issue
Nov 24, 2023
- added a bug fix for accessng obspy mapping in classmethod - added methods called operating_input_units, operating_output_units. - These are like units_in and units_out, but the "operating" units are congnizant of whether the filter is to be applied via multiplication, or "unapplied" via division.
kkappler
added a commit
that referenced
this issue
Dec 7, 2023
- tests are passing for filters and want to have this commit staged so updates to main can be merged in - however, changes as outlined in issue #173 to be made before PR
kkappler
added a commit
that referenced
this issue
Dec 7, 2023
- added a direction to FilterBase ("forward" or "inverse") - this defines "application_operation" and "correction_operation", which are normally "multiply" and "divide" respectively. - Added some mixins to ChannelResponseFilter from FilterBase to avoid duplicating these methods, as it didn't feel appropriate to extend FilterBase to ChannelResponseFilter.
kkappler
added a commit
that referenced
this issue
Dec 7, 2023
kkappler
added a commit
that referenced
this issue
Dec 8, 2023
- add method to channel_response_filter that gets list of filters to remove, allowing for booleans include_decimation, include_delay - this method also returns a list of the indices of the filters that will be removed - the index list can be used to flip only the booleans associated with filters that are removed
This was referenced Dec 12, 2023
Merged
kujaku11
added a commit
that referenced
this issue
Dec 15, 2023
- Fix issue #173 - Set distinct direction of filters - Filters are only for channel response - `applied` true and false flipped to align with FDSN
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
There is an inconsistency in the filter attributes. When creating an MTH5 from FDSN, filters in a ChannelTS have their "applied" attributes set to False by default, and "applied" is set to True when the instrument response is removed.
The final units of the time series, after application of
remove_instrument_response
(for example in mth5_driver example), seem correct but they correspond to filter "units_in" of the last stage, not filter "units_out".Intuition says:
The reason for this apparent dissonance: FDSN filters use input and output units to describe the role of the transducer according to the signal flow from external source field to archived digital record. E.g. Here is a snippet of XML from CAS04 response
Removal of the instrument response does not correspond to "applying" these filters, but to "unapplying" them. Thus we divide by the filter response (invert the filter operation), rather than multiply by it (apply the filter). This is not only an FDSN convention -- manufacturer calibration functions for magnetometers for example are provided in terms of V/nT, i.e. Output units over Input units.
We should keep this convention inside
FilterBase
.Complications may arise if we want to work with more general filters. Consider the case where a user wishes to remove the channel response and then band pass the data. The band pass filter is not in the filters list and there is currently no record keeping for this filter if it is requested. For the MTH5 to be self-describing we could add a record of this band pass operation to any channel against which it has been applied (and it would seem that the "applied" boolean should be marked as True in whatever list is tracking whether it was applied). Note This means that the "state" of the filters is non-uniform at the end of the process, the response filters have applied=False, the band pass has applied=True.
The role of the filters needs to be scoped before it winds up trying to do too much. The "Filters Group" in the MTH5 is concerned with calibration and system response information and not with processing filters. Other filters, such as band pass, and notch, etc. were not intended to be handled here. The other filters captured by using a workflow pattern (such as the processing class used to compute TFs and FCs). This can be as simple as a dictionary that contains the parameters to pass to the calibration operation, which can include specification of the band pass filter.
Desired Outcome
Approaches / Conventions / Considerations
Filters are defined with the forward (multiplicative) operation being in the direction of the signal process flow from the fundamental measurement to the archived digital record.
The "applied" attribute reflects whether the forward application of the filter has been carried out on the ChannelTS
Units In /Units Out needed for all filters?
Tasks:
Players on the stage are:
"applied", "units_in", "units_out", and the units associated with the
ChannelTS
objectapplied
to have the valueTrue
, or change it's name tocorrection_applied
correction_operation
, default value isdivide
include_decimation
andinclude_time_delay
Nice to have:
Still More:
While tweaking tests, it seems that units handling is still not 100% there
While ChannelTS metadata does have a units field, it doesn't interact with mt_metadata.utils.units
add method to channel.py that returns the units object so that this can be compared with filter units in/out.
add mixins from FilterBase to ChannelResponseFilter
Manage setting "forward"/"inverse", "divide"/"multiply" attributes that defines their relationship in only one place.
The attributes direction (one of ["forward", "inverse"]) and the attribute application_operation in FilterBase
maybe superfluous. We could probably do away with one of them (direction).
operation="divide"
kwarg ints_filters.RemoveInstrumentResponse.remove_instrument_response
. Note that the filters do have "units_in", and "units_out", which makes them directional, and to the doc has now been added that they are by definition applied by multiplying in frequency domain. It would certainly be cleaner to remove any reference to filter "direction", "application_operation", "correction_operation". I will make a branch that does this and we can merge it in if we like.fix_issue_173a
branch and a corresponding branch in mth5. We need to decide if we will use fix_issue_173 or fix_issue_173a branch in the final PR.The text was updated successfully, but these errors were encountered: