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

Generalize and Improve Annotation Handling #32

Merged
merged 36 commits into from
Sep 8, 2022
Merged

Conversation

aromanielloNTIA
Copy link
Member

@aromanielloNTIA aromanielloNTIA commented Aug 15, 2022

Overview

This update enhances the implementation of SigMF annotation segments and measurement metadata. Hard-coded metadata values are removed to ensure that actions provide accurate metadata as required by the SigMF specifications. In general, metadata is no longer pulled by the annotation classes directly from a measurement_result dictionary, which makes annotation use more explicit. Additionally, actions no longer need to store metadata in the measurement_result.

These changes extend the functionality of the annotation classes, allowing better support for future actions. For example, the annotation class TimeDomainDetection can now support time domain captures other than raw IQ captures.

All existing actions are updated to make use of the refactored metadata classes.

Changelog

  • Add imports to scos_actions/metadata/annotations/__init__.py so that all annotations are importable from scos_actions.metadata.annotations
  • Refactor annotations as Python dataclasses. This makes their definitions easier to write, more legible, and more consistent.
  • Refactor annotations so that they don't depend on specific keys being present in measurement_result dictionaries, instead requiring each metadata value to be specified as a parameter for the annotation instance
  • Adds the AnnotationSegment base dataclass, which annotations can be built from. This base class implements the required and optional keys associated with any possible SigMF annotation segment. This replaces the old Metadata base class.
  • Change the MeasurementMetadata class to a dataclass, and implement previously missing keys which are part of the SigMF specification. Also adds checks that required values are specified.
  • Update all annotation classes to:
    • Inherit from AnnotationSegment
    • Contain all possible keys defined in the related SigMF specification (many were previously missing)
    • Check that all keys required by the related SigMF specification are given values
    • Add docstrings describing the parameters of the annotation segment.
  • Note on CalibrationAnnotation:
    • Keep old functionality: has a parameter to specify the gain_sensor key, which is actually not a part of the ntia-sensor specification
  • Notes on FrequencyDomainDetection annotation:
    • Moved from ../fft_annotation.py to ../frequency_domain_detection.py
    • Keep old functionality: prepend "fft_" to the detector parameter, to make it easy to use with the new generalized detector implementation
  • Notes on TimeDomainDetection:
    • Moved from ../time_domain_annotation.py to ../time_domain_detection.py
  • Update actions (acquire_single_freq_fft, acquire_single_freq_tdomain_iq, and acquire_stepped_freq_tdomain_iq) and sigm_builder for the above changes. Note that the changes now require a classification key in the measurement metadata (required by the SigMF specification), and all three actions are hard-coded to record their results as "UNCLASSIFIED"

@aromanielloNTIA aromanielloNTIA added the enhancement New feature or request label Aug 15, 2022
@aromanielloNTIA aromanielloNTIA marked this pull request as draft August 16, 2022 20:49
@aromanielloNTIA
Copy link
Member Author

Saving as draft for now- let's save this for after the incoming SigMF-NS-NTIA updates happen, and make all the corresponding SigMF-related updates in one PR.

Base automatically changed from dsp-refactor to master August 23, 2022 16:48
@aromanielloNTIA aromanielloNTIA changed the title Generalize Time and Frequency Domain Annotations Generalize and Improve Annotation Handling Aug 30, 2022
@aromanielloNTIA
Copy link
Member Author

Leaving out ProbabilityDistributionAnnotation (which was waiting for its ntia-algorithm spec to be finalized) makes this PR ready for review. Will add in the ProbabilityDistributionAnnotation with #27 when it's ready.

@aromanielloNTIA aromanielloNTIA marked this pull request as ready for review August 30, 2022 18:01
Copy link
Contributor

@dboulware dboulware left a comment

Choose a reason for hiding this comment

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

Looks good. I think we need to revisit the metadata handling, but I'll create issues to do it at a later date.

@@ -99,11 +97,19 @@ def execute(self, schedule_entry, task_id) -> dict:
measurement_result["description"] = self.description
measurement_result["sigan_cal"] = self.sigan.sigan_calibration_data
measurement_result["sensor_cal"] = self.sigan.sensor_calibration_data
measurement_result["classification"] = "UNCLASSIFIED"
Copy link
Contributor

Choose a reason for hiding this comment

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

Would this be better to pull from YAML?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, definitely better than hard-coding this in.

Copy link
Member Author

Choose a reason for hiding this comment

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

Commenting here to keep this traceable: #41 is this issue

@@ -115,6 +115,7 @@ def __call__(self, schedule_entry_json, task_id):
measurement_result["name"] = self.name
measurement_result["sigan_cal"] = self.sigan.sigan_calibration_data
measurement_result["sensor_cal"] = self.sigan.sensor_calibration_data
measurement_result["classification"] = "UNCLASSIFIED"
Copy link
Contributor

Choose a reason for hiding this comment

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

Would this be better to pull from YAML?

@@ -191,6 +188,7 @@ def execute(self, schedule_entry, task_id) -> dict:
measurement_result["measurement_type"] = MeasurementType.SINGLE_FREQUENCY.value
measurement_result["sigan_cal"] = self.sigan.sigan_calibration_data
measurement_result["sensor_cal"] = self.sigan.sensor_calibration_data
measurement_result["classification"] = "UNCLASSIFIED"
Copy link
Contributor

Choose a reason for hiding this comment

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

Might be better to pull from YAML, but we can add an issue to do that.

@@ -243,8 +241,19 @@ def description(self):
def get_sigmf_builder(self, measurement_result) -> SigMFBuilder:
sigmf_builder = super().get_sigmf_builder(measurement_result)
for i, detector in enumerate(self.fft_detector):
fft_annotation = FrequencyDomainDetectionAnnotation(
detector.value, i * self.fft_size, self.fft_size
fft_annotation = FrequencyDomainDetection(
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a lot of arguments. I agree that it helps to make it explicit what it requires but I think we need to revisit this later. Maybe later we move to a builder or fluent interface.

Copy link
Member Author

Choose a reason for hiding this comment

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

Agreed that we should improve this later. Hopefully along with more metadata handling improvements overall (maybe tied to addressing #29?)

@@ -99,11 +97,19 @@ def execute(self, schedule_entry, task_id) -> dict:
measurement_result["description"] = self.description
measurement_result["sigan_cal"] = self.sigan.sigan_calibration_data
measurement_result["sensor_cal"] = self.sigan.sensor_calibration_data
measurement_result["classification"] = "UNCLASSIFIED"
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above, but we don't need to fix now.

@@ -39,15 +36,50 @@ def __call__(self, schedule_entry, task_id):
def get_sigmf_builder(self, measurement_result) -> SigMFBuilder:
Copy link
Contributor

Choose a reason for hiding this comment

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

This is still relying on measuremen_result containing everything. Don't need to fix now, but we should create an issue and revisit.

Copy link
Member Author

Choose a reason for hiding this comment

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

Figuring out how/when to rely on measurement_result should be a main priority of #40

assert (
self.freq_lower_edge is not None and self.freq_upper_edge is not None
), err_msg
# Define SigMF key names
Copy link
Contributor

Choose a reason for hiding this comment

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

We should probably move to using a json schema in the future, but not needed for now.

@dboulware dboulware merged commit 4b3ead1 into master Sep 8, 2022
@aromanielloNTIA aromanielloNTIA deleted the generalize-annotate branch September 8, 2022 18:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants