Skip to content

Adding the inverse tool FEObservationOperator#1217

Open
ConnorMallon wants to merge 11 commits into
gridap:masterfrom
ConnorMallon:master
Open

Adding the inverse tool FEObservationOperator#1217
ConnorMallon wants to merge 11 commits into
gridap:masterfrom
ConnorMallon:master

Conversation

@ConnorMallon

Copy link
Copy Markdown
Contributor

Adding the differentiable FEObservationOperator object that efficiently evaluates FEFunctions at a set of observation points.

Copilot AI review requested due to automatic review settings February 10, 2026 00:44
@codecov

codecov Bot commented Feb 10, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 67.64706% with 33 lines in your changes missing coverage. Please review.
✅ Project coverage is 88.96%. Comparing base (1df55b8) to head (56d7632).
⚠️ Report is 890 commits behind head on master.

Files with missing lines Patch % Lines
src/Inverse/FEObservationOperators.jl 67.32% 33 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1217      +/-   ##
==========================================
- Coverage   89.04%   88.96%   -0.08%     
==========================================
  Files         213      215       +2     
  Lines       27529    27631     +102     
==========================================
+ Hits        24513    24582      +69     
- Misses       3016     3049      +33     
Flag Coverage Δ
drivers 0.00% <0.00%> (ø)
extensions 0.00% <0.00%> (ø)
tests 88.96% <67.64%> (-0.08%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

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

Adds a new Inverse submodule to Gridap that introduces a differentiable FEObservationOperator for evaluating FE functions at arbitrary observation points, and wires it into the package exports and dependencies.

Changes:

  • Added Inverse module with FEObservationOperator implementation and ChainRulesCore rrule.
  • Exposed FEObservationOperator through Exports.jl and loaded the new module from src/Gridap.jl.
  • Added ChainRulesCore dependency and introduced initial tests for the operator/adjoint.

Reviewed changes

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

Show a summary per file
File Description
test/InverseTests/runtests.jl New test entrypoint for inverse-related tests.
test/InverseTests/InverseTests.jl Adds an adjoint check for FEObservationOperator via finite differences.
src/Inverse/Inverse.jl Introduces the Inverse submodule, imports, and exports for inverse tooling.
src/Inverse/FEObservationOperators.jl Implements SequentialFEObservationOperator, its evaluation, and rrule pullbacks.
src/Gridap.jl Loads the new Inverse submodule.
src/Exports.jl Publishes FEObservationOperator from Inverse.
Project.toml Adds ChainRulesCore to deps/compat.

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

Comment thread src/Inverse/FEObservationOperators.jl Outdated
Comment thread src/Inverse/Inverse.jl Outdated
Comment thread test/InverseTests/InverseTests.jl Outdated
Comment thread test/InverseTests/runtests.jl
Comment thread src/Inverse/Inverse.jl Outdated
Comment on lines +26 to +34
struct SequentialFEObservationOperator{T} <: FEObservationOperator
obs_points::AbstractVector{<:Point}
fe_space::SingleFieldFESpace
cell_to_points::Gridap.Arrays.Table
cells_w_points
cell_to_weights::AbstractVector{Matrix{T}}
filtered_indices::Vector{Int32}
cache
end

Copilot AI Feb 10, 2026

Copy link

Choose a reason for hiding this comment

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

SequentialFEObservationOperator leaves cells_w_points and cache untyped, which makes these fields Any and can introduce type instability and dynamic dispatch in a performance-sensitive operator. Consider adding concrete field types (or additional type parameters) for these fields (e.g. cells_w_points::AbstractVector{<:Integer} / Vector{Int32} and cache::NTuple{4,Any} or a fully-parametric cache type).

Copilot uses AI. Check for mistakes.
@Antoinemarteau

Copy link
Copy Markdown
Collaborator

Out of curiosity, why did this require creating a new module, and why is it called Inverse ?

@ConnorMallon

ConnorMallon commented Mar 3, 2026

Copy link
Copy Markdown
Contributor Author

Out of curiosity, why did this require creating a new module, and why is it called Inverse ?

Because it is a differentiable tool made specifically for inverse problems. In the future, the plan is to put more things in this module. But also,it is true that the forward version in of itself is more general than just inverse problems - happy to take suggestions for the naming and location.

@JordiManyer

Copy link
Copy Markdown
Member

@ConnorMallon lets try to move this forward.

  • We should pull from master so that everything is set in Gridap v0.20.
  • I think InverseProblems is a better module name?
  • Why Sequential? I assume there is a distributed version, but the sequential one should not be called that.

The biggest issue I am not sure what this new structure is supposed to accomplish. The name is not too indicative, and it just seems to be a counterpart of the Interpolable object.
The current Interpolable fixes the object you want to interpolate, whereas this new object fixes the points you want to interpolate at. Could this just be a new type of interpolator cache?
You also seem to re-write a bunch of code (the point classification code) that is already in compute_cell_points_from_vector_of_points.

I think this needs to be split into two separate things:

  • The point classification should just be part of Interplation.jl, maybe we can create some sort of InterpolablePointCache, that can then be fed to an interpolable to make interpolation efficient (repeatedly).
  • Then a second part that could somehow englobe the whole chainrules stuff.

Am I missing something?

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.

4 participants