Skip to content

Remove case sensitivity for variable dimensions#660

Merged
peverwhee merged 3 commits into
developfrom
fix-dimension-case-sensitivity
Jun 3, 2025
Merged

Remove case sensitivity for variable dimensions#660
peverwhee merged 3 commits into
developfrom
fix-dimension-case-sensitivity

Conversation

@peverwhee
Copy link
Copy Markdown
Collaborator

@peverwhee peverwhee commented Jun 2, 2025

Updates metadata_table.py parsing to lowercase the following fields:

  • local name
  • standard name
  • dimensions

Context: I ran into an issue where dimension variable standard names were not being found if they contain >=1 upper case letter.

To reproduce, modify any dimension in any of the capgen tests to be mixed case. The test run will fail with this message: Could not find dimension...

User interface changes?: No

Copy link
Copy Markdown
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.

Is this the only place where a lowercase version of dimension names is needed? Or should this be done much deeper down in capgen when parsing the metadata files?

Copy link
Copy Markdown
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'm okay with this if it solves whatever problem caused this work (there is no issue or problem mentioned in the PR description). It is a standard accessing routine.

@peverwhee
Copy link
Copy Markdown
Collaborator Author

@gold2718 - I added a bit more context to the PR description!

@climbfuji - a good question. I'll look into how tricky it is to lower all of the variable standard names and dimensions at parse time.

@peverwhee peverwhee requested a review from gold2718 June 3, 2025 16:09
@peverwhee
Copy link
Copy Markdown
Collaborator Author

@climbfuji @gold2718

Dom was right to suggest an "earlier" change to lowercase - there was at least one other path through the code that resulted in the same error. I updated the parser to return both dimensions and standard names in lower case.

One question for y'all and for @dustinswales :

Thanks to Dustin's recent PR, numerical dimensions are OK (like (horizontal_dimension, 2)) but what about numerical indices for dimensions (like (2:horizontal_dimension))? Is that allowed? If so, it's not working and I'll open an issue.

@peverwhee peverwhee requested a review from climbfuji June 3, 2025 16:17
@climbfuji
Copy link
Copy Markdown
Collaborator

@climbfuji @gold2718

Dom was right to suggest an "earlier" change to lowercase - there was at least one other path through the code that resulted in the same error. I updated the parser to return both dimensions and standard names in lower case.

One question for y'all and for @dustinswales :

Thanks to Dustin's recent PR, numerical dimensions are OK (like (horizontal_dimension, 2)) but what about numerical indices for dimensions (like (2:horizontal_dimension))? Is that allowed? If so, it's not working and I'll open an issue.

I think that (2:horizontal_dimension) is allowed.

Copy link
Copy Markdown
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.

Still looks okay to me.

@dustinswales
Copy link
Copy Markdown
Member

@climbfuji @gold2718
Dom was right to suggest an "earlier" change to lowercase - there was at least one other path through the code that resulted in the same error. I updated the parser to return both dimensions and standard names in lower case.
One question for y'all and for @dustinswales :
Thanks to Dustin's recent PR, numerical dimensions are OK (like (horizontal_dimension, 2)) but what about numerical indices for dimensions (like (2:horizontal_dimension))? Is that allowed? If so, it's not working and I'll open an issue.

I think that (2:horizontal_dimension) is allowed.

I concur.

Copy link
Copy Markdown
Member

@dustinswales dustinswales left a comment

Choose a reason for hiding this comment

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

There changes look good to me.
One question. I wonder if we still need this change I made with your changes to the parser?

@peverwhee
Copy link
Copy Markdown
Collaborator Author

@dustinswales

Good question! I removed that line from your previous PR and also added a line in the parser to make the local names lowercase as well.

@peverwhee peverwhee merged commit 9254784 into develop Jun 3, 2025
19 checks passed
@peverwhee peverwhee deleted the fix-dimension-case-sensitivity branch June 3, 2025 22:50
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