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.

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

Add fieldvector unit tests, update rcompare #2072

Merged
merged 1 commit into from
Nov 5, 2024
Merged

Conversation

charleskawczynski
Copy link
Member

This PR adds a fieldvector unit test, which was extracted from #2070, where the unit tests passed but a test in ClimaLand broke.

It's still not clear to me whether this is exercising broadcasting over different fieldvectors types with the same propertynames, or something different. Either way, we should probably test for that, plus, we could update rcompare to handle fieldvectors with different propertyname orders.

@charleskawczynski charleskawczynski changed the title Add fieldvector unit tests Add fieldvector unit tests, update rcompare Nov 4, 2024
@charleskawczynski
Copy link
Member Author

I think updating rcompare to handle different propertyname orders is okay, since users can easily test typeof(x) == typeof(y), but I figure we can make these a keyword option so that users can pick which they want. By default, though, the == will be strict, I think that's probably the safest option.

@charleskawczynski
Copy link
Member Author

This fails when combined with #2070, so this should help us figure out what is wrong with #2070.

@charleskawczynski
Copy link
Member Author

Ok, I think I've identified what is being exercised: nested fieldvectors with different propertyname ordering. I'll update the PR

@charleskawczynski charleskawczynski merged commit 36837c2 into main Nov 5, 2024
11 of 12 checks passed
@charleskawczynski charleskawczynski deleted the ck/fv_unit branch November 5, 2024 18:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant