GEOPY-3045: Missing unit conversion for Frequency systems - #464
GEOPY-3045: Missing unit conversion for Frequency systems#464domfournier wants to merge 4 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR addresses GEOPY-3045 by ensuring Frequency-Domain EM (FEM/FDEM) frequencies are correctly interpreted when the dataset’s frequency unit is not plain Hertz, by introducing unit-aware conversion in survey construction and updating the synthetic FDEM survey metadata/test expectations.
Changes:
- Store synthetic FDEM frequencies in kHz (instead of Hz) and record the intended frequency unit on the survey.
- Apply
unit_conversionwhen building FDEM survey frequency arguments and when populating SimPEG survey_frequencies. - Update the FEM driver test uncertainty scaling to reflect the new frequency unit convention.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| tests/run_tests/driver_airborne_fem_test.py | Adjusts uncertainty scaling to account for frequency unit changes in synthetic FDEM surveys. |
| simpeg_drivers/utils/synthetics/surveys/frequency_domain/fdem.py | Updates synthetic FDEM frequency configuration to kHz and attempts to set the survey’s frequency unit. |
| simpeg_drivers/components/factories/survey_factory.py | Converts FEM channel frequencies using unit_conversion before constructing SimPEG sources. |
| simpeg_drivers/components/data.py | Ensures SimPEG survey _frequencies reflect unit-converted channel values. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #464 +/- ##
===========================================
+ Coverage 90.25% 90.27% +0.01%
===========================================
Files 113 113
Lines 7022 7021 -1
Branches 867 867
===========================================
Hits 6338 6338
+ Misses 467 466 -1
Partials 217 217
🚀 New features to boost your workflow:
|
| if hasattr(survey, "_frequencies"): | ||
| survey._frequencies = self.entity.channels # pylint: disable=protected-access | ||
| survey._frequencies = ( # pylint: disable=protected-access | ||
| np.asarray(self.entity.channels) / self.params.unit_conversion | ||
| ) |
There was a problem hiding this comment.
_frequencies is converted to Hz here, but it looks like _naturalsource_arguments still passes the raw data.entity.channels values to SourcesFactory. For units other than Hz does this mean the survey and its sources would end up using different values for frequencies in different places, e.g. for 0.9 kHz, source.frequency and survey._frequency_dict would be left as 0.9 while survey._frequencies would be 900.0?
There was a problem hiding this comment.
(I'm not 100% sure this is how everything will work but I thought I should bring it up just in case)
GEOPY-3045 - Missing unit conversion for Frequency systems