Add scalar_fieldmatrix#2289
Closed
imreddyTeja wants to merge 1 commit intomainfrom
Closed
Conversation
imreddyTeja
force-pushed
the
tr/fieldmatrix-functions-for-jacobian
branch
2 times, most recently
from
67fd30e to
65b036a
Compare
imreddyTeja
force-pushed
the
tr/fieldmatrix-functions-for-jacobian
branch
from
65b036a to
3f81735
Compare
imreddyTeja
force-pushed
the
tr/fieldmatrix-functions-for-jacobian
branch
from
2405aca to
02bb025
Compare
imreddyTeja
commented
Apr 28, 2025
imreddyTeja
force-pushed
the
tr/fieldmatrix-functions-for-jacobian
branch
from
d91cf59 to
e1c1c22
Compare
| eltype(x.parent) | ||
| inner_type_ignore_adjoint(x::DataLayouts.AbstractData) = eltype(x) | ||
| inner_type_ignore_adjoint(x::Adjoint) = typeof(x.parent) | ||
| inner_type_ignore_adjoint(x) = typeof(x) |
Member
There was a problem hiding this comment.
Should this also support when x is a broadcasted object that contains an Adjoint?
Member
Author
There was a problem hiding this comment.
It currently is never used where x could be broadcasted, but maybe it should be added so this could be used elsewhere? Although this should be used with caution, because it is a bit of a hack (it is only used with axis vectors)
imreddyTeja
force-pushed
the
tr/fieldmatrix-functions-for-jacobian
branch
3 times, most recently
from
03c0a76 to
ef4ead4
Compare
imreddyTeja
force-pushed
the
tr/fieldmatrix-functions-for-jacobian
branch
2 times, most recently
from
2c6c054 to
52cf757
Compare
Add a function to convert a FieldMatrix where each matrix entry has an eltype of some struct into a FieldMatrix where each entry has an eltype of a scalar. Add additional tests for scalar_matrixfields Use @test_all in tests Make suggested changes to tests and field_name_dict.jl Revert unrolled_findfirst Clean up field matrix tests and add support for DiagonalMatrixRows CamelCase struct name Clean up tests and get_scalar_keys wip backup Minimal working with allocs WIP1 WIP more allocs fix Assorted cleanup Fix dx/dx case reduce code duplication; fix example Add gpu test further cleanup, extend diagonalrow fix names test and comments Add docs docs bugfix remvoe bad refs fix docs formatting
imreddyTeja
force-pushed
the
tr/fieldmatrix-functions-for-jacobian
branch
from
52cf757 to
13633ca
Compare
dennisYatunin
approved these changes
Jun 3, 2025
Member
dennisYatunin
left a comment
dennisYatunin
left a comment
There was a problem hiding this comment.
Thank you Teja! We should eventually try to simplify the scalar indexing logic, as it's ended up very entangled with the AxisTensor internals, but I think you've captured every case we could possibly support. The new section in the docs is also helpful. This will be really useful for implementing the sparse autodiff Jacobian in ClimaAtmos.
charleskawczynski
approved these changes
Jun 18, 2025
Member
There was a problem hiding this comment.
It's nice to see that this works. My only feedback is that a lot of those functions are pretty big with seemingly lots of edge cases. So, I question how maintainable it is. That said, I'm fine with it if you are.
Also, it might be nice to test out with atmos before merging (both for correctness and compile time).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
closes #2306
This PR adds
scalar_fieldmatrix, a function which takes in a field matrix, and returns a field matrix with keys that are associated with matrix fields of either uniform scaling or columnwisebandmatrices of scalars.This also makes the
get_indexfor field matrices return a field instead of a broadcasted object when the keys used to index the field matrix will result in a columnwisebandmatrices of scalars.get_internal_entry, which is called byget_indexfor FieldMatrices, is not complete, but works for everything thatscalar_fieldmatrixneeds. Any indexing that was previously supported is still supported. An example of something that could be added, is if an entry is a tuple of axistensors, indexing into only the axis tensor components.