Skip to content

GEOPY-3045: Missing unit conversion for Frequency systems - #464

Open
domfournier wants to merge 4 commits into
developfrom
GEOPY-3045
Open

GEOPY-3045: Missing unit conversion for Frequency systems#464
domfournier wants to merge 4 commits into
developfrom
GEOPY-3045

Conversation

@domfournier

@domfournier domfournier commented Aug 18, 2026

Copy link
Copy Markdown
Collaborator

GEOPY-3045 - Missing unit conversion for Frequency systems

@domfournier
domfournier marked this pull request as ready for review August 18, 2026 22:47
Copilot AI lite review requested due to automatic review settings August 18, 2026 22:47
@github-actions github-actions Bot changed the title GEOPY-3045 GEOPY-3045: Missing unit conversion for Frequency systems Aug 18, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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_conversion when 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.

Comment thread simpeg_drivers/utils/synthetics/surveys/frequency_domain/fdem.py
Comment thread tests/run_tests/driver_airborne_fem_test.py
@codecov

codecov Bot commented Aug 19, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.27%. Comparing base (8d7c87e) to head (81f68a0).

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              
Files with missing lines Coverage Δ
simpeg_drivers/components/data.py 95.60% <100.00%> (ø)
...peg_drivers/components/factories/survey_factory.py 94.92% <100.00%> (ø)
...ivers/electromagnetics/frequency_domain/options.py 91.54% <100.00%> (+1.40%) ⬆️
...eg_drivers/electromagnetics/time_domain/options.py 100.00% <100.00%> (ø)
...s/natural_sources/apparent_conductivity/options.py 100.00% <100.00%> (ø)
...rivers/natural_sources/magnetotellurics/options.py 98.21% <100.00%> (-0.04%) ⬇️
simpeg_drivers/natural_sources/tipper/options.py 100.00% <100.00%> (ø)
.../utils/synthetics/surveys/frequency_domain/fdem.py 88.88% <100.00%> (+0.31%) ⬆️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Comment on lines 335 to +338
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
)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

_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?

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

(I'm not 100% sure this is how everything will work but I thought I should bring it up just in case)

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