Capgen cleanup #493

peverwhee · 2023-09-01T16:29:21Z

Summary

This PR contains mostly clean-up for readability and clarity.

Description

Update CODEOWNERS file
Add optional database object output for capgen
Code cleanup

User interface changes?: Yes
Can now include optional return_db argument to capgen to return a database for use by host model.

Addresses #380

Testing:
All tests passing except for unit conversion test that is expected to fail.

gold2718

Yow, lots of nice cleanup!
For the record, I shed no tears for Python2.

scripts/ccpp_state_machine.py

gold2718 · 2023-09-01T18:31:14Z

scripts/ccpp_suite.py

+            input_vars = [set(), set(), set()] # leaves, arrays, leaf elements
+            inout_vars = [set(), set(), set()] # leaves, arrays, leaf elements
+            output_vars = [set(), set(), set()] # leaves, arrays, leaf elements


No more pirates?

gold2718 · 2023-09-01T18:36:40Z

scripts/parse_tools/parse_checkers.py

@@ -12,7 +12,7 @@

 ########################################################################

-_UNITS_RE = re.compile(r"^[^/@#$%^&*()\|<>\[\]{}?,.]+$")
+_UNITS_RE = re.compile(r"^[^/!@#$%^&*=()\|<>\[\]{}?,.]+$")


Really? What do examples that use these look like?

Since it's impossible to decipher regular expressions like this, it might be good to add a few examples above what we want to match. This would also help addressing @gold2718's question.

After staring at this for a minute, I think we can actually clean this up a bit if it makes sense with something like:

unitless_regex = "1" unit_regex = "[a-zA-Z]+[-]?[0-9]+" units_regex = f"^({unit_regex}(\s{unit_regex})?|{unitless_regex})$" # >>> print(units_regex) # ^([a-zA-Z]+[-]?[0-9]+(\s[a-zA-Z]+[-]?[0-9]+)?|1)$ _UNITS_RE = re.compile(units_regex)

Testing locally and on regex101 looks to allow only what we want minus the exponents with leading 0's (like m s-01) but that's easy enough to add in, but I was just curious if this was a reasonable replacement first?

@climbfuji @nusbaume thoughts on @mwaxmonsky 's regex cleanup?

Did a little more testing and I missed two common cases so I think a more flushed out version could be:

unitless_regex = "1" non_leading_zero_num = "[1-9]\d*" negative_non_leading_zero_num = f"[-]{non_leading_zero_num}" unit_exponent = f"({negative_non_leading_zero_num}|{non_leading_zero_num})" unit_regex = f"[a-zA-Z]+{unit_exponent}?" units_regex = f"^({unit_regex}(\s{unit_regex})*|{unitless_regex})$" # >>> print(units_regex) # ^([a-zA-Z]+([-][1-9]\d*|[1-9]\d*)?(\s[a-zA-Z]+([-][1-9]\d*|[1-9]\d*)?)*|1)$ _UNITS_RE = re.compile(units_regex)

I'm not as familiar with the backwards compatibility needed for the whitespace support but I think we can leave that out of the regex and pass the proposed units str through python's strip() function to remove all surrounding whitespace and then pass it to _UNITS_RE.match(...).

I like @mwaxmonsky's solution!

Update pushed. Just let me know if the new version has any issues!

Also on the documentation front, I noticed that there's a few doctest tests which pass in the units and error on invalid strings. Would we be duplicating documentation by putting in examples outside the doctest for check_units() right below the regex declaration?

@mwaxmonsky has pushed his regex updates, which is both cleaner and easier to decipher!

dustinswales

@peverwhee Looks good to me. Thanks for this contribution.
(And good riddance to python v2!!!)

scripts/ccpp_state_machine.py

climbfuji

A few minor comments below. Since this PR removes Python2 support (yay), is this mentioned or encoded somewhere, for example on a top-level README.md and/or CMakeLists.txt?

climbfuji · 2023-09-06T20:15:37Z

scripts/ccpp_database_obj.py

+              datatable.xml file created by capgen.
+        """
+        metadata_tables = {}
+        host_name = "cam"


I see that host_name is hardcoded to cam. What is the plan for making this configurable in the future?

@peverwhee Can you comment on this please?

@climbfuji Since the creating a database object from a file is not yet implemented, this host name is not currently doing anything. I did change it to something more general in the interim, but I think long-term, it will be parsed from the datafile.

scripts/ccpp_state_machine.py

scripts/ccpp_suite.py

climbfuji · 2023-09-06T20:38:55Z

scripts/parse_tools/parse_checkers.py

@@ -12,7 +12,7 @@

 ########################################################################

-_UNITS_RE = re.compile(r"^[^/@#$%^&*()\|<>\[\]{}?,.]+$")
+_UNITS_RE = re.compile(r"^[^/!@#$%^&*=()\|<>\[\]{}?,.]+$")


Since it's impossible to decipher regular expressions like this, it might be good to add a few examples above what we want to match. This would also help addressing @gold2718's question.

mkavulich · 2023-09-11T20:38:15Z

RE: python2 I'm pretty sure python 2 support was de-facto ended long ago (there are a lot of python-3 specific constructs like f-strings deep in the framework, and have been there for more than a year). I'm not sure that requires any specific notes.

mwaxmonsky

Looks great! Just two small syntax things I noticed.

scripts/metavar.py

Merge main into feature/capgen (merge after #493)

climbfuji

Thanks for addressing my comments and for simplifying the regular expressions!

climbfuji · 2023-10-04T22:37:12Z

scripts/ccpp_database_obj.py

@@ -56,7 +56,7 @@ def db_from_file(self, run_env, database_file):
              datatable.xml file created by capgen.
        """
        metadata_tables = {}
-        host_name = "cam"
+        host_name = "host"


mwaxmonsky

Looks good to me!

climbfuji · 2023-10-12T18:44:59Z

@peverwhee @nusbaume This has been approved by a bunch of people. @nusbaume Can you look at your previous comments and approve if ok, so that we can merge this? Thanks!

Courtney Peverley and others added 5 commits August 21, 2023 13:34

initial stab at cleanup-only PR

6a6cc0d

cleanup and fixes

3d75c77

bugfixes

52626d0

add code owners

3c6a9e2

code cleanup

1be9b3d

peverwhee self-assigned this Sep 1, 2023

peverwhee requested review from climbfuji and gold2718 as code owners September 1, 2023 16:29

peverwhee requested review from mwaxmonsky and dustinswales September 1, 2023 16:47

gold2718 approved these changes Sep 1, 2023

View reviewed changes

gold2718 mentioned this pull request Sep 2, 2023

Testing updates #494

Merged

dustinswales approved these changes Sep 6, 2023

View reviewed changes

scripts/ccpp_state_machine.py Outdated Show resolved Hide resolved

climbfuji reviewed Sep 6, 2023

View reviewed changes

climbfuji mentioned this pull request Sep 11, 2023

Merge main into feature/capgen (merge after #493) #499

Merged

mwaxmonsky reviewed Sep 18, 2023

View reviewed changes

scripts/metavar.py Outdated Show resolved Hide resolved

scripts/metavar.py Outdated Show resolved Hide resolved

climbfuji added a commit that referenced this pull request Sep 21, 2023

Merge pull request #499 from NCAR/main

5f355cc

Merge main into feature/capgen (merge after #493)

peverwhee mentioned this pull request Sep 25, 2023

Convert old python strings to f-strings #501

Open

merge up to head of feature/capgen

55e4c59

peverwhee requested review from grantfirl and mkavulich as code owners September 26, 2023 01:45

Courtney Peverley and others added 3 commits September 29, 2023 16:55

generalize host

c8ad82d

change to fstrings

44ea46f

Cleaning up units regex.

3f5c06f

peverwhee requested review from climbfuji and mwaxmonsky October 2, 2023 16:05

climbfuji approved these changes Oct 4, 2023

View reviewed changes

mwaxmonsky approved these changes Oct 5, 2023

View reviewed changes

nusbaume requested review from nusbaume and removed request for nusbaume October 12, 2023 21:42

nusbaume approved these changes Oct 12, 2023

View reviewed changes

peverwhee merged commit c2c2aa0 into NCAR:feature/capgen Oct 12, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Capgen cleanup #493

Capgen cleanup #493

peverwhee commented Sep 1, 2023 •

edited

Loading

gold2718 left a comment

gold2718 Sep 1, 2023

gold2718 Sep 1, 2023

climbfuji Sep 6, 2023

mwaxmonsky Sep 26, 2023

peverwhee Sep 27, 2023

mwaxmonsky Sep 27, 2023

mwaxmonsky Sep 28, 2023

climbfuji Sep 28, 2023

mwaxmonsky Sep 29, 2023

mwaxmonsky Sep 29, 2023

peverwhee Oct 2, 2023

dustinswales left a comment

climbfuji left a comment

climbfuji Sep 6, 2023

climbfuji Sep 28, 2023

peverwhee Sep 29, 2023

climbfuji Sep 6, 2023

mkavulich commented Sep 11, 2023

mwaxmonsky left a comment

climbfuji left a comment

climbfuji Oct 4, 2023

mwaxmonsky left a comment

climbfuji commented Oct 12, 2023

Capgen cleanup #493

Capgen cleanup #493

Conversation

peverwhee commented Sep 1, 2023 • edited Loading

Summary

Description

gold2718 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dustinswales left a comment

Choose a reason for hiding this comment

climbfuji left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkavulich commented Sep 11, 2023

mwaxmonsky left a comment

Choose a reason for hiding this comment

climbfuji left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mwaxmonsky left a comment

Choose a reason for hiding this comment

climbfuji commented Oct 12, 2023

peverwhee commented Sep 1, 2023 •

edited

Loading