(towards #3252) improve value range env variable#3355
Conversation
…in invalid files in other Makefiles).
…he value ranges.
…range_env_variable
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #3355 +/- ##
=======================================
Coverage 99.95% 99.95%
=======================================
Files 386 386
Lines 54187 54187
=======================================
Hits 54165 54165
Misses 22 22 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
IT passed, ready for review. |
|
Note that I had a README update after IT, they will obviously not affect the results. |
sergisiso
changed the title
sergisiso
left a comment
sergisiso
left a comment
There was a problem hiding this comment.
Thanks for fixing this @hiker I like the . way more than the __ for navigating modules-regions-varaibles. And getting rid of any ambiguities with valid fortran names. I just have minor comments about documentation and testing.
| run: compile | ||
| # Note that if the output line is not found, the make will abort with error | ||
| ./value_range_check 2>&1| grep "PSyData: Variable a_fld has the invalid value" | ||
| ./value_range_check 2>&1| grep "PSyData: Variable 'a_fld%data' has the invalid value" |
There was a problem hiding this comment.
Great.
Maybe we should update ".github/workflows/compilation.yml" to do
F90=gfortran make -C examples run instead of compile?
(I tried and most succeeded but there are some errors, but I reckon we can bypass those since they are broken also for command line execution, and we need to fix them at some point anyway)
| :3.3 A value less than or equal to 3.3 | ||
| 1.1: A value greater than or equal to 1.1 | ||
|
|
||
| The syntax for the environment variable is one of: |
There was a problem hiding this comment.
Should this mention that is ;-separated entries?
| The syntax for the environment variable is one of: | ||
|
|
||
| ``PSYVERIFY__module__kernel__variable`` | ||
| ``PSY_VALUE_RANGE="module.kernel.variable=...`` |
There was a problem hiding this comment.
Now that this is domain-agnostic we probably shouldn't use 'kernel'. I see the lib implementation uses "region" which is a better name, but it is also not documented anywhere what this means. I believe is the region_name that any PSyData can give to its application region? Could you add some explanation about this in the lines below.
2 participants
Changes the implementation to use a single environment variable
PSY_VALUE_RANGEto specify all variables, which adds flexible support for structures etc (field%data=...).It does not address the fact that now a user needs to know the 'internal' name (after lowering) of a variable that is passed to a kernel.