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

Capgen in SCM: Multiple instances of local_name in Group Cap #631

Open
wants to merge 5 commits into
base: develop
Choose a base branch
from

Conversation

dustinswales
Copy link
Collaborator

@dustinswales dustinswales commented Jan 29, 2025

Description:
Allow for multiple instances of the same local_name being used in the Group cap for two situations:
a) with different standard_names
b) in different DDTs .

User interface changes?: No

Fixes: #629

Testing:
Added to var_compatibility_test to exercise feature.

This PR contains changes included in #630

Copy link
Collaborator

@gold2718 gold2718 left a comment

Choose a reason for hiding this comment

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

I have a suggesting for modifying the test a bit but it looks okay for scalar vars. It would be nice to also implement a DDT test as @peverwhee suggested.

test/var_compatibility_test/test_host_data.meta Outdated Show resolved Hide resolved
@dustinswales
Copy link
Collaborator Author

I have a suggesting for modifying the test a bit but it looks okay for scalar vars. It would be nice to also implement a DDT test as @peverwhee suggested.

@gold2718 Thanks for the suggestions. I added them to the tests.

@peverwhee There are some various tests in the pipeline for DDTs (e.g. #637 #640), albeit stressing different things. I could extend those or add here. Thoughts?

Copy link
Collaborator

@climbfuji climbfuji left a comment

Choose a reason for hiding this comment

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

Thanks for fixing this!

@peverwhee
Copy link
Collaborator

@peverwhee There are some various tests in the pipeline for DDTs (e.g. #637 #640), albeit stressing different things. I could extend those or add here. Thoughts?

@dustinswales I vote whatever's easiest for you!

Copy link
Collaborator

@gold2718 gold2718 left a comment

Choose a reason for hiding this comment

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

👍

Comment on lines +1680 to +1681
# Check if local_name exists in Group. If applicable, Create new
# variable with uniquie name. There are two instances when new names are
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
# Check if local_name exists in Group. If applicable, Create new
# variable with uniquie name. There are two instances when new names are
# Check if local_name exists in Group. If applicable, create new
# variable with unique name. There are two instances when new names are

@@ -41,6 +41,13 @@
kind = kind_phys
intent = in
top_at_one = True
[ scalar_var ]
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is probably my capgen ignorance showing, but I'm not seeing how these new test variables are checking the new logic. They aren't in DDTs and they aren't duplicate names with different standard names, unless I'm missing something?.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@mkavulich it's the variable naming in the caps. The test now uses same local_name for 3 different standard_names. To see this, run the var_compatabilty_test and look at the suite cap in ct_build/ccpp, you will see that each instance of scalar_var in the Cap has been renamed to a unique name, scalar_var1, Scalar_var2, and Scalar_var3, respectively.

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.

5 participants