Throw clearer error message when integer dimension indices are present; remove known ddt case sensitivity#662
Conversation
peverwhee
requested review from
climbfuji,
dustinswales,
gold2718,
grantfirl,
mkavulich and
mwaxmonsky
as code owners
climbfuji
left a comment
climbfuji
left a comment
There was a problem hiding this comment.
This all looks reasonable to me, thanks for adding the doctests
mkavulich
left a comment
mkavulich
left a comment
There was a problem hiding this comment.
Looks good to me, just a minor fix suggestion
| errmsg = "'{}' is an invalid dimension name" | ||
| raise CCPPError(errmsg.format(item)) | ||
| if info_msg: | ||
| errmsg = f"'{item} is an invalid dimension name; {info_msg}" |
There was a problem hiding this comment.
| errmsg = f"'{item} is an invalid dimension name; {info_msg}" | |
| errmsg = f"'{item}' is an invalid dimension name; {info_msg}" |
There was a problem hiding this comment.
good catch! fixed!
gold2718
left a comment
gold2718
left a comment
There was a problem hiding this comment.
I am confused by what is allowed and what is not. What purpose do integers serve and when are they allowed? Is this documented?
| # in an ugly spot | ||
| best = self.__line_max - 1 | ||
| if len(outstr) > best: | ||
| if len(outstr) > best + 1: |
There was a problem hiding this comment.
Does this work if self.__line_max == 132 (Fortran maximum)?
There was a problem hiding this comment.
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
- 113 characters, I get an error trying to do line 218 of fortran_write.py -
line_continue = outstr[best+1:].lstrip()[0] != '!' - longer than 113 characters, the generated code for
ccpp_physics_suite_variablestries 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'
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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!
| 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'] |
There was a problem hiding this comment.
Why is this okay? Isn't 1 an integer dimension index?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
peverwhee
changed the title
gold2718
left a comment
gold2718
left a comment
There was a problem hiding this comment.
I think this is okay. I wrote a test to check.
Add test for writing long strings
4 participants
Add error handling for integer dimension indices; remove case sensitivity for known DDTs
Description
User interface changes?: No
Closes #661 - numerical dimension indices error in capgen
Closes #664 - long standard name bug
Testing:
test removed: n/a
unit tests: all PASS, added two new doctests
system tests:
manual testing: