Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Enable indexing FieldVectors with column indices #1441

Merged
merged 1 commit into from
Sep 19, 2023
Merged

Enable indexing FieldVectors with column indices #1441

merged 1 commit into from
Sep 19, 2023

Conversation

LenkaNovak
Copy link
Contributor

@LenkaNovak LenkaNovak commented Sep 1, 2023 •
edited
Loading

Closes #1442

  • Code follows the style guidelines OR N/A.
  • Unit tests are included OR N/A.
  • Code is exercised in an integration test OR N/A.
  • Documentation has been added/updated OR N/A.

@LenkaNovak LenkaNovak mentioned this pull request Sep 2, 2023
Closed
9 tasks
@LenkaNovak LenkaNovak marked this pull request as ready for review September 2, 2023 01:50
@LenkaNovak LenkaNovak force-pushed the ln/getindex-fieldvector branch from 374cc48 to ace210f Compare September 2, 2023 01:51
@LenkaNovak LenkaNovak force-pushed the ln/getindex-fieldvector branch 2 times, most recently from 00e41a8 to 18a2ceb Compare September 7, 2023 05:39
akshaysridhar
akshaysridhar approved these changes Sep 7, 2023
View reviewed changes
Copy link
Member

@akshaysridhar akshaysridhar left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for adding this method.

@akshaysridhar
Copy link
Member

bors try

bors bot added a commit that referenced this pull request Sep 7, 2023
@bors
Try #1441:
310bd4a
charleskawczynski
charleskawczynski reviewed Sep 7, 2023
View reviewed changes
Copy link
Member

@charleskawczynski charleskawczynski left a comment

Choose a reason for hiding this comment

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

Can we add an inference test to this? I don't think that the FieldVector outer constructor is type stable

@bors
Copy link
Contributor

bors bot commented Sep 7, 2023

try

Build succeeded!

The publicly hosted instance of bors-ng is deprecated and will go away soon.

If you want to self-host your own instance, instructions are here.
For more help, visit the forum.

If you want to switch to GitHub's built-in merge queue, visit their help page.

@LenkaNovak
Copy link
Contributor Author

don't think that the FieldVector outer constructor is type stable

Hi @charleskawczynski , thanks for reviewing. And sure we can add an interface test! What exactly do you have in mind? There are other functions in the same file that test methods dispatched on FieldVector, so I assumed it was safe.

@charleskawczynski charleskawczynski mentioned this pull request Sep 8, 2023
Merged
@charleskawczynski
Copy link
Member

Hi @charleskawczynski , thanks for reviewing. And sure we can add an interface test! What exactly do you have in mind? There are other functions in the same file that test methods dispatched on FieldVector, so I assumed it was safe.

It's safe, but calling that particular outer constructor is not type-stable. Please see CliMA/ClimaAtmos.jl#2079 for an attempted fix where we're hitting the same problem with the old TC code. It has a huge performance penalty-- note that it's completely fine to use once or twice (outside of step!), but (at least) the constructor that calls promote_type(map(RecursiveArrayTools.recursive_bottom_eltype, values)...) should really not be put inside step!.

@LenkaNovak LenkaNovak force-pushed the ln/getindex-fieldvector branch from 18a2ceb to 1081697 Compare September 8, 2023 21:40
charleskawczynski
charleskawczynski approved these changes Sep 8, 2023
View reviewed changes
Copy link
Member

@charleskawczynski charleskawczynski left a comment

Choose a reason for hiding this comment

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

Alright, this new version is fully inferred and only allocates 32 bytes (per column). So I think this can work.

Thanks for fixing this @LenkaNovak!

@charleskawczynski
Copy link
Member

Actually, I'm not sure that this is working, see this build, which fails 🤔

@LenkaNovak
Copy link
Contributor Author

Awesome, thank you for your help, @charleskawczynski !

Actually, I'm not sure that this is working, see this build, which fails 🤔

Ahh, is that indexing a FieldVector of FieldVectors?

@LenkaNovak LenkaNovak force-pushed the ln/getindex-fieldvector branch from 1081697 to 5da30b0 Compare September 8, 2023 23:00
@charleskawczynski
Copy link
Member

Actually, I think the breaking part is due to how we handled names in the old TC code. I think this should be fine, so long we add a test that propertynames(field_vector) == propertynames(field_vector[colidx]).

@LenkaNovak
Copy link
Contributor Author

bors try

bors bot added a commit that referenced this pull request Sep 8, 2023
@bors
Try #1441:
0eb0b09
@bors
Copy link
Contributor

bors bot commented Sep 9, 2023

try

Build failed:

@LenkaNovak LenkaNovak force-pushed the ln/getindex-fieldvector branch from 9175abd to 7e24dc3 Compare September 9, 2023 01:05
@LenkaNovak
Copy link
Contributor Author

bors try

bors bot added a commit that referenced this pull request Sep 9, 2023
@bors
Try #1441:
1041f1d
@LenkaNovak LenkaNovak force-pushed the ln/getindex-fieldvector branch from 7e24dc3 to 43106cc Compare September 9, 2023 01:51
@LenkaNovak
Copy link
Contributor Author

bors try

@bors
Copy link
Contributor

bors bot commented Sep 9, 2023

try

Already running a review

@bors
Copy link
Contributor

bors bot commented Sep 9, 2023

try

Build failed:

@LenkaNovak
Copy link
Contributor Author

bors try

bors bot added a commit that referenced this pull request Sep 9, 2023
@bors
Try #1441:
469dee8
bors bot added a commit to CliMA/ClimaAtmos.jl that referenced this pull request Sep 9, 2023
@bors @charleskawczynski
Merge #2079
f751e69 
2079: Try to fix FieldVector inference r=charleskawczynski a=charleskawczynski

This PR is an attempt to fix the very bad remaining inference failure in the TC step! function, by calling a different FieldVector constructor to avoid the inference failure in (what I assume is `promote_type(map(RecursiveArrayTools.recursive_bottom_eltype, values)...)`).

Inference currently looks like this in the EDMF flame graph:
![Screen Shot 2023-09-08 at 11 09 07 AM](https://github.com/CliMA/ClimaAtmos.jl/assets/1880641/c3e984cf-a42f-42ab-bfe1-a306604854fd)

Let's see if this PR has any impact.

cc `@LenkaNovak,` `@akshaysridhar` 

This is why I suggested adding JET tests in CliMA/ClimaCore.jl#1441, it will work, but it will be impractically slow. This flame graph is for a column, I imagine the sphere would be much worse.

Co-authored-by: Charles Kawczynski <[email protected]>
@bors
Copy link
Contributor

bors bot commented Sep 9, 2023

try

Build failed:

@LenkaNovak LenkaNovak force-pushed the ln/getindex-fieldvector branch 2 times, most recently from d51abc7 to f9ef25a Compare September 18, 2023 03:32
@LenkaNovak
Copy link
Contributor Author

bors try

bors bot added a commit that referenced this pull request Sep 18, 2023
@bors
Try #1441:
e90be36
@bors
Copy link
Contributor

bors bot commented Sep 18, 2023

try

Build failed:

@LenkaNovak LenkaNovak force-pushed the ln/getindex-fieldvector branch from f9ef25a to ede17f2 Compare September 18, 2023 16:16
@LenkaNovak
Copy link
Contributor Author

bors try

bors bot added a commit that referenced this pull request Sep 18, 2023
@bors
Try #1441:
ba7340c
@bors
Copy link
Contributor

bors bot commented Sep 18, 2023

try

Build failed:

@LenkaNovak
VF getindex func + test
13ffca3 
move func to indices.jl

prior format

make output a FieldVector

rev + type stable

Co-authored-by: @charleskawczynski  <[email protected]>

add name test

rm parent idx in test

convert to Array fix

fix cuda test with @allowscalar

alloc test only for CPU

timeout fix
@LenkaNovak LenkaNovak force-pushed the ln/getindex-fieldvector branch from ede17f2 to 13ffca3 Compare September 18, 2023 20:21
@LenkaNovak
Copy link
Contributor Author

bors try

bors bot added a commit that referenced this pull request Sep 18, 2023
@bors
Try #1441:
2938e93
@bors
Copy link
Contributor

bors bot commented Sep 18, 2023

try

Build succeeded!

The publicly hosted instance of bors-ng is deprecated and will go away soon.

If you want to self-host your own instance, instructions are here.
For more help, visit the forum.

If you want to switch to GitHub's built-in merge queue, visit their help page.

@LenkaNovak
Copy link
Contributor Author

bors r+

bors bot added a commit that referenced this pull request Sep 18, 2023
@bors @LenkaNovak
Merge #1441
5193ea7 
1441: Enable indexing FieldVectors with column indices r=LenkaNovak a=LenkaNovak



Co-authored-by: LenkaNovak <[email protected]>
@bors
Copy link
Contributor

bors bot commented Sep 18, 2023

Build failed:

@LenkaNovak
Copy link
Contributor Author

bors r+

@bors
Copy link
Contributor

bors bot commented Sep 19, 2023

Build succeeded!

The publicly hosted instance of bors-ng is deprecated and will go away soon.

If you want to self-host your own instance, instructions are here.
For more help, visit the forum.

If you want to switch to GitHub's built-in merge queue, visit their help page.

@bors bors bot merged commit 82c2454 into main Sep 19, 2023
6 checks passed
@bors bors bot deleted the ln/getindex-fieldvector branch September 19, 2023 00:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@akshaysridhar akshaysridhar akshaysridhar approved these changes

@charleskawczynski charleskawczynski charleskawczynski approved these changes

@simonbyrne simonbyrne Awaiting requested review from simonbyrne

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Enable indexing FieldVectors with column indices
3 participants
@LenkaNovak @akshaysridhar @charleskawczynski