(Closes #3259) shorten generated names for basis/diff-basis arrays #3264
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #3264 +/- ##
=======================================
Coverage 99.95% 99.95%
=======================================
Files 376 376
Lines 53485 53495 +10
=======================================
+ Hits 53463 53473 +10
Misses 22 22 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
This PR shortens the generated names used for various variables in order to avoid hitting compiler limitations as reported by Tom in #3259. Since we're no longer basing the names on existing variable names (which must be unique) I've had to increase the use of tags to ensure we cope in the face of name collisions. |
| self._boundary_dofs.append(self.BoundaryDofs(op_arg, bc_fs)) | ||
|
|
||
| def invoke_declarations(self): | ||
| def invoke_declarations(self) -> None: |
There was a problem hiding this comment.
Maybe this is a stylistic choice, but is the "-> None" needed when there is no return? None of my tooling complains with or without
There was a problem hiding this comment.
I believe some tools do complain - @hiker mentioned this.
There was a problem hiding this comment.
I suppose this is mypy, I am using ty for typechecking and it does not complain. Anyway, if we need something I suggest NoReturn https://docs.python.org/3/library/typing.html#typing.NoReturn , the method does not return None.
There was a problem hiding this comment.
I think it does. NoReturn indicates that a function never returns while a function without an explicit return does return None (at least, that's what pylint complains about if you ever have a function where it thinks you've missed a return statement).
src/psyclone/lfric.py
Outdated
| symbol = self.symtab.find_or_create_tag( | ||
| basis, | ||
| symbol_type=DataSymbol, datatype=UnsupportedFortranType( | ||
| f"real(kind=r_def), allocatable :: {basis}{dims}" |
There was a problem hiding this comment.
As you did somewhere else, you will need to do:
new_name = self.symtab.next_available_name(basis)
if you embed the name in the datatype (as it may end up being different)
| kind = const.QUADRATURE_TYPE_MAP["gh_quadrature_xyoz"]["kind"] | ||
| for name in self.qr_weight_vars["xyoz"]: | ||
| self.symtab.find_or_create( | ||
| self.symtab.find_or_create_tag( |
There was a problem hiding this comment.
new_name = self.symtab.next_available_name(...)
| :returns: a shortened form of the name. | ||
| ''' | ||
| new_parts = [] |
There was a problem hiding this comment.
Maybe its a pain to do not that you already changed lots of tests, but some of the generated code is more cryptic now, have you considered doing:
if len(name) < threshold:
return name
to keep already short names descriptive?
There was a problem hiding this comment.
Ah! That's a good idea. I've done some googling and realised that the standard actually mandates a maximum length. F2008:
while in F95 it was 31 apparently.
|
|
||
| # Split name string to find any_*_space ID and create a short name as | ||
| # "<start>" + "spc" + "ID" | ||
| # "<start>" + "s" + "ID" |
There was a problem hiding this comment.
Is this function still necessary now?, we could do:
self._shorten_arg_name(self.orig_name + "_" + arg.name)}
But if you decide not to remove the short_name do we really need to keep the _short_name and _mangled_name as state, it may be safer to compute them in the property method.
There was a problem hiding this comment.
I've kept this one but have, as you suggest, removed the internal state in favour of dynamic computation.
| f"space '{self._orig_name}'") | ||
|
|
||
| @staticmethod | ||
| def _shorten_arg_name(name: str) -> str: |
There was a problem hiding this comment.
Maybe just "_shorten_name", it doesn't use the fact is an arg.
No description provided.