Skip to content

GEOPY-2799: Implement MVI with PDE inversion#375

Open
domfournier wants to merge 29 commits intodevelopfrom
GEOPY-2799
Open

GEOPY-2799: Implement MVI with PDE inversion#375
domfournier wants to merge 29 commits intodevelopfrom
GEOPY-2799

Conversation

@domfournier
Copy link
Copy Markdown
Collaborator

@domfournier domfournier commented Apr 13, 2026

GEOPY-2799 - Implement MVI with PDE inversion

Copilot AI review requested due to automatic review settings April 13, 2026 20:24
@github-actions github-actions bot changed the title GEOPY-2799 GEOPY-2799: Implement MVI with PDE inversion Apr 13, 2026
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

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 introduces support for a new “magnetic vector pde” workflow (forward + inversion) and integrates it into joint inversion drivers/tests, alongside updates to rotated-regularization handling and related plumbing.

Changes:

  • Added new magnetic_vector_pde driver package (forward/inversion/options) and new PDE UI JSON assets.
  • Updated joint drivers/tests to exercise MVI PDE in joint survey / cross-gradient contexts.
  • Refactored rotated-regularization operator handling (degrees vs radians) and adjusted nested/simulation factory logic for remanence mapping.

Reviewed changes

Copilot reviewed 42 out of 42 changed files in this pull request and generated 6 comments.

Show a summary per file
File Description
simpeg_drivers/potential_fields/magnetic_vector_pde/* Adds PDE forward/inversion drivers and options for the new method.
simpeg_drivers-assets/uijson/magnetic_vector_pde_*.ui.json Adds UI schemas for PDE forward/inversion runs.
simpeg_drivers/components/models.py Extends vector-model logic to include PDE inversion type; adjusts gradient rotation handling.
simpeg_drivers/components/factories/* Allows factories (survey/source/receiver/simulation/directives) to recognize “magnetic vector pde”.
simpeg_drivers/joint/* Adjusts joint regularization/model handling to work with global mesh + PDE cases.
simpeg_drivers/utils/regularization.py Updates rotated operator setup to accept degrees and convert internally.
simpeg_drivers/utils/nested.py Ensures local tile mapping uses 3 components when remanence mapping is present.
tests/run_tests/driver_mvi_pde_test.py Adds an integration-style test file for PDE forward+inversion.
tests/run_tests/driver_joint_surveys_test.py Parametrizes joint surveys MVI test to include PDE driver/options.
tests/run_tests/driver_joint_cross_gradient_test.py Adds a rotated-gradient joint cross-gradient test and tweaks inv run behavior.
pyproject.toml + conda lock files Pins mira-simpeg and refreshes environment locks accordingly.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

benk-mira and others added 6 commits April 13, 2026 14:50
# Conflicts:
#	environments/py-3.12-linux-64-dev.conda.lock.yml
#	environments/py-3.12-linux-64.conda.lock.yml
#	environments/py-3.12-win-64-dev.conda.lock.yml
#	environments/py-3.12-win-64.conda.lock.yml
#	environments/py-3.13-linux-64-dev.conda.lock.yml
#	environments/py-3.13-linux-64.conda.lock.yml
#	environments/py-3.13-win-64-dev.conda.lock.yml
#	environments/py-3.13-win-64.conda.lock.yml
#	environments/py-3.14-linux-64-dev.conda.lock.yml
#	environments/py-3.14-linux-64.conda.lock.yml
#	environments/py-3.14-win-64-dev.conda.lock.yml
#	environments/py-3.14-win-64.conda.lock.yml
#	py-3.12.conda-lock.yml
#	py-3.13.conda-lock.yml
#	py-3.14.conda-lock.yml
@codecov
Copy link
Copy Markdown

codecov bot commented Apr 14, 2026

Codecov Report

❌ Patch coverage is 90.36697% with 21 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.21%. Comparing base (13c5823) to head (8904734).

Files with missing lines Patch % Lines
simpeg_drivers/driver.py 83.33% 5 Missing and 3 partials ⚠️
.../potential_fields/magnetic_vector_pde/inversion.py 81.48% 3 Missing and 2 partials ⚠️
...rs/potential_fields/magnetic_vector_pde/forward.py 70.00% 2 Missing and 1 partial ⚠️
...peg_drivers/components/factories/source_factory.py 77.77% 1 Missing and 1 partial ⚠️
simpeg_drivers/joint/driver.py 93.33% 1 Missing and 1 partial ⚠️
...drivers/components/factories/directives_factory.py 75.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop     #375      +/-   ##
===========================================
+ Coverage    90.05%   90.21%   +0.15%     
===========================================
  Files          127      131       +4     
  Lines         6368     6497     +129     
  Branches       795      809      +14     
===========================================
+ Hits          5735     5861     +126     
+ Misses         428      427       -1     
- Partials       205      209       +4     
Files with missing lines Coverage Δ
simpeg_drivers/__init__.py 88.88% <ø> (ø)
...g_drivers/components/factories/receiver_factory.py 89.90% <100.00%> (ø)
...peg_drivers/components/factories/simpeg_factory.py 92.59% <ø> (ø)
...drivers/components/factories/simulation_factory.py 98.90% <100.00%> (+0.09%) ⬆️
...peg_drivers/components/factories/survey_factory.py 95.04% <100.00%> (ø)
simpeg_drivers/components/models.py 91.44% <100.00%> (ø)
...impeg_drivers/joint/joint_cross_gradient/driver.py 92.85% <100.00%> (+2.23%) ⬆️
simpeg_drivers/joint/joint_surveys/driver.py 93.05% <100.00%> (+0.86%) ⬆️
simpeg_drivers/joint/joint_surveys/options.py 92.30% <100.00%> (ø)
...s/potential_fields/magnetic_vector_pde/__init__.py 100.00% <100.00%> (ø)
... and 11 more
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Copy Markdown
Contributor

@benk-mira benk-mira left a comment

Choose a reason for hiding this comment

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

Some questions/comments in the review. Also.. I wasn't expecting a whole new driver/options/ui.json combo. 1. Is the old method worth keeping around? (Will people want to choose between them on a case-by-case basis?) 2. Could this be written as an option in the mvi ui.json to switch from pde/integral equation?

regularizations = super().get_regularization()
reg_list, multipliers = self._overload_regularization(regularizations)
# Trick the drivers by swapping the inversion_mesh and models
# such that the regularization uses the global mesh
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.

This must be the fix for GEOPY-2656?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Yes, in big part. Also, because this new PDE Driver has a special regularization, we need to rely on each sub-inversion to generate their own reg. So the trick here is to collect all the models, interpolate onto the global mesh, then pass it back to the sub inversion to form their reg

inducing_field_strength: float | FloatData
inducing_field_inclination: float | FloatData
inducing_field_declination: float | FloatData
models: VectorModelPDEOptions
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.

Why no reference inclination/declination like in the regular mvi?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Because we currently don't have mechanics to enforce direction in Cartesian space. This new PDE formalulation doesn't allow to switch to spherical, so we can't allow it, for now.

if any(
k in inversion_type
for k in ["direct current", "induced polarization", "gravity", "magnetic"]
for k in ["direct current", "induced polarization", "gravity", "magnetic "]
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.

why is the space needed? Seems like you could omit.. "magnetic scalar", "magnetic vector", and "magnetic vector pde" should all get captured without any confusion with "electromagnetic" as far as I can tell.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I don't think so, no. Because we do k in inversion_type, then "magnetic" in "electromagnetics" is True, which would take all of the EM methods. The space prevents that.

Copy link
Copy Markdown
Collaborator Author

@domfournier domfournier left a comment

Choose a reason for hiding this comment

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

Fair point that there is a lot of overlap, but there is enough for us to keep both around, for now. Main difference is that the PDE can't do reference angles, and has a special VectorAmplitude reg.
I think it would be possible to do reference angles in Cartesian space, but I ll need more time to develop. For now it's just simpler to have a second.

regularizations = super().get_regularization()
reg_list, multipliers = self._overload_regularization(regularizations)
# Trick the drivers by swapping the inversion_mesh and models
# such that the regularization uses the global mesh
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Yes, in big part. Also, because this new PDE Driver has a special regularization, we need to rely on each sub-inversion to generate their own reg. So the trick here is to collect all the models, interpolate onto the global mesh, then pass it back to the sub inversion to form their reg

inducing_field_strength: float | FloatData
inducing_field_inclination: float | FloatData
inducing_field_declination: float | FloatData
models: VectorModelPDEOptions
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Because we currently don't have mechanics to enforce direction in Cartesian space. This new PDE formalulation doesn't allow to switch to spherical, so we can't allow it, for now.

if any(
k in inversion_type
for k in ["direct current", "induced polarization", "gravity", "magnetic"]
for k in ["direct current", "induced polarization", "gravity", "magnetic "]
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I don't think so, no. Because we do k in inversion_type, then "magnetic" in "electromagnetics" is True, which would take all of the EM methods. The space prevents that.

Co-authored-by: benk-mira <81254271+benk-mira@users.noreply.github.com>
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