Skip to content

Throw clearer error message when integer dimension indices are present; remove known ddt case sensitivity #662

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

Merged
mkavulich merged 6 commits into from
Jul 9, 2025
Merged

Throw clearer error message when integer dimension indices are present; remove known ddt case sensitivity #662

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
2d1761c
fix two bugs
peverwhee Jun 25, 2025
f28b5d6
add error handling for integer dimension indices
peverwhee Jun 25, 2025
6e7098d
back out line length fix and address review comments
peverwhee Jun 26, 2025
faff223
another stab at fixing line length cusp bug
peverwhee Jun 27, 2025
4f7230d
Add test for writing long strings
Jun 27, 2025
17442fe
Merge pull request #17 from gold2718/small-bugfixes
peverwhee Jun 30, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion scripts/fortran_tools/fortran_write.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -210,7 +210,7 @@ def write(self, statement, indent_level, continue_line=False):
# This is probably a bad situation so we have to break
# in an ugly spot
best = self.__line_max - 1
if len(outstr) > best:
if len(outstr) > best + 1:

@gold2718 gold2718 Jun 26, 2025

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Does this work if self.__line_max == 132 (Fortran maximum)?

@peverwhee peverwhee Jun 26, 2025

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Looking into this more, I'm going to back this out and open an issue because it's more complicated than I thought (as you've noticed).

Due to quotation marks and indenting, when a standard name is

  1. 113 characters, I get an error trying to do line 218 of fortran_write.py - line_continue = outstr[best+1:].lstrip()[0] != '!'
  2. longer than 113 characters, the generated code for ccpp_physics_suite_variables tries to split the standard name string like:
            variable_list(3) =                                                                    &
                 'tendency_of_dry_air_enthalpy_at_constant_pressure_due_to_shortwave_radiation_adjusted_by_air_pressure_thicknesse&
&s'

@gold2718 gold2718 Jun 26, 2025

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

            variable_list(3) =                                                                    &
                 'tendency_of_dry_air_enthalpy_at_constant_pressure_due_to_shortwave_radiation_adjusted_by_air_pressure_thicknesse&
&s'

Is that not okay? I thought that is how you were supposed to split a character context across lines.

@peverwhee peverwhee Jun 26, 2025

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

oh! well, then, I suppose we're good on that front. i'm showing my fortran ignorance! so it's just the first issue that i'll look into a bit more tomorrow.

@peverwhee peverwhee Jun 27, 2025

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Thanks again @gold2718

I took another stab at fixing the issue where the line is exactly equal to the max line length and doesn't need to be continued. If you have other ideas on how to fix it, I'm all ears!

line_continue = '&'
# end if
# end if
Expand Down
10 changes: 5 additions & 5 deletions scripts/metadata_table.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -205,7 +205,7 @@ def parse_metadata_file(filename, known_ddts, run_env, skip_ddt_check=False):
meta_tables.append(new_table)
table_titles.append(ntitle)
if new_table.table_type == 'ddt':
known_ddts.append(ntitle)
known_ddts.append(ntitle.lower())
# end if
else:
errmsg = 'Duplicate metadata table, {}, at {}:{}'
Expand Down Expand Up @@ -540,7 +540,7 @@ def __init_from_file(self, known_ddts, run_env, skip_ddt_check=False):
raise CCPPError(self.__pobj.error_message)
# end if
if self.table_type == "ddt":
known_ddts.append(self.table_name)
known_ddts.append(self.table_name.lower())
# end if
if self.__dependencies is None:
self.__dependencies = []
Expand Down Expand Up @@ -882,7 +882,7 @@ def __init_from_file(self, table_name, table_type, known_ddts, module_name,
self.title, start_ctx))
# end if
if self.header_type == "ddt":
known_ddts.append(self.title)
known_ddts.append(self.title.lower())
# end if
# Initialize our ParseSource parent
super().__init__(self.title, self.header_type, self.__pobj)
Expand Down Expand Up @@ -954,11 +954,11 @@ def parse_variable(self, curr_line, known_ddts, skip_ddt_check=False):
pval_str = prop[1]
if ((pname == 'type') and
(not check_fortran_intrinsic(pval_str, error=False))):
if skip_ddt_check or pval_str in known_ddts:
if skip_ddt_check or pval_str.lower() in known_ddts:
if skip_ddt_check:
register_fortran_ddt_name(pval_str)
# end if
pval = pval_str
pval = pval_str.lower()
pname = 'ddt_type'
else:
errmsg = "Unknown DDT type, {}".format(pval_str)
Expand Down
30 changes: 27 additions & 3 deletions scripts/parse_tools/parse_checkers.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,13 @@ def check_dimensions(test_val, prop_dict, error, max_len=0):
>>> check_dimensions("hi_mom", None, True) #doctest: +IGNORE_EXCEPTION_DETAIL
Traceback (most recent call last):
CCPPError: 'hi_mom' is invalid; not a list
>>> check_dimensions(["1:dim1", "dim2name"], None, True) #doctest: +IGNORE_EXCEPTION_DETAIL
Traceback (most recent call last):
CCPPError: '1:dim1 is an invalid dimension name; integer dimension indices not supported
>>> check_dimensions(["ccpp_constant_one:1", "dim2name"], None, True)
['ccpp_constant_one:1', 'dim2name']

@gold2718 gold2718 Jun 26, 2025

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Why is this okay? Isn't 1 an integer dimension index?

@peverwhee peverwhee Jun 26, 2025

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

a valid question! The problem I ran into is that sometimes the parse checker is called after the "ccpp_constant_one:" is prepended to the integer dimension (which I believe we're allowing) and sometimes it's called before.

So, the metadata could specify dimensions = (2) and we'd end up calling check_dimensions on ccpp_constant_one:2

I thought about diving deeper into figuring out what the specifics were of when we're calling what, but decided that ccpp_constant_one:1 is the same as saying 1 and left it at that.

@gold2718 gold2718 Jun 26, 2025

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Here is what is confusing: If "integer dimension indices not supported", how is the 1, an integer okay? Also, when did we decide it is okay to use integers for array extents?

@peverwhee peverwhee Jun 26, 2025

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

basically, it's only ok if you do ccpp_constant_one:1. any other use of integer indices isn't ok. And that's because of my earlier comment - to appease the checker.

As for when we started allowing integer sizes, i believe @dustinswales brought that in recently for his work in SCM - #652

"""
info_msg = None
if not isinstance(test_val, list):
if error:
raise CCPPError("'{}' is invalid; not a list".format(test_val))
Expand All @@ -123,10 +129,24 @@ def check_dimensions(test_val, prop_dict, error, max_len=0):
# end if
# Check possible dim styles (a, a:b, a:, :b, :, ::, a:b:c, a::c)
tdims = [x.strip() for x in isplit if len(x) > 0]
starts_at_one = False
if len(tdims) > 0 and tdims[0] == 'ccpp_constant_one':
starts_at_one = True
# end if
is_int = False
for tdim in tdims:
# Check numeric value first
try:
valid = isinstance(int(tdim), int)
is_int = isinstance(int(tdim), int)
# Allow integer dimensions, but not indices
if is_int:
valid = starts_at_one or len(tdims) == 1
if not valid:
info_msg = 'integer dimension indices not supported'
# end if
else:
valid = False
# end if
except ValueError as ve:
# Not an integer, try a Fortran ID
valid = check_fortran_id(tdim, None,
Expand All @@ -147,8 +167,12 @@ def check_dimensions(test_val, prop_dict, error, max_len=0):
# End try
if not valid:
if error:
errmsg = "'{}' is an invalid dimension name"
raise CCPPError(errmsg.format(item))
if info_msg:
errmsg = f"'{item} is an invalid dimension name; {info_msg}"

@mkavulich mkavulich Jun 26, 2025

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Suggested change
errmsg = f"'{item} is an invalid dimension name; {info_msg}"
errmsg = f"'{item}' is an invalid dimension name; {info_msg}"

@peverwhee peverwhee Jun 26, 2025

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

good catch! fixed!

else:
errmsg = f"'{item}' is an invalid dimension name"
# end if
raise CCPPError(errmsg)
else:
test_val = None
# end if
Expand Down