Skip to content

Conversation

PrometheusPi
Copy link
Member

This PR adds a helper class that converts the c++ code string from PICMI free density formula to a sympy expression and enables evaluating it.

@PrometheusPi PrometheusPi added this to the 0.9.0 / next stable milestone Sep 26, 2025
@PrometheusPi PrometheusPi added component: tools scripts, python libs and CMake CI:no-compile CI is skipping compile/runtime tests but runs PICMI tests PICMI pypicongpu and picmi related labels Sep 26, 2025
@PrometheusPi PrometheusPi force-pushed the add_python_lib_to_read_picmi_density branch from 1e68ddd to e48404d Compare September 26, 2025 15:28
@PrometheusPi
Copy link
Member Author

@chillenzer I would be happy to implement any idea to shorten this parser.

Copy link
Contributor

@ikbuibui ikbuibui left a comment

Choose a reason for hiding this comment

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

My main comment is that I think the ternary op handling and parsing can be done in init, maybe use sympy.Piecewise for the ternary op, and then an evaluate will become quite straightforward.
You will need to provide the symbol e.g. y to init though.
This can be useful depending on how this is used.
Offline discussion: This feature is a stopgap till we start storing sympy expressions instead of recreating them from C++. This code is not intended to be kept in long term.
Other small comments below

from picmi_density_reader import cpp_fct_reader

__all__ = ["FindTime", "MemoryCalculator", "FieldIonization", "FLYonPICRateCalculationReference"]
__all__ = ["FindTime", "MemoryCalculator", "FieldIonization", "FLYonPICRateCalculationReference", "cpp_fct_reader"]
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
__all__ = ["FindTime", "MemoryCalculator", "FieldIonization", "FLYonPICRateCalculationReference", "cpp_fct_reader"]
__all__ = ["FindTime", "MemoryCalculator", "FieldIonization", "FLYonPICRateCalculationReference", "CppFctReader"]

Doesn't follow naming scheme.

Copy link
Contributor

Choose a reason for hiding this comment

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

Personally I also prefer Fn for functions instead of Fct


if __name__ == "__main__":
# load pypicongpu.json, convert json to dict and extract equation for density
file = open("pypicongpu.json")
Copy link
Contributor

Choose a reason for hiding this comment

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

missing corresponding file close.

Copy link
Member Author

Choose a reason for hiding this comment

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

I added a comment how to generate this file

Copy link
Contributor

Choose a reason for hiding this comment

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

I was wondering if you need a call to file close at the end of main to prevent resource leaks.
https://stackoverflow.com/questions/7395542/is-explicitly-closing-files-important

Copy link
Member Author

Choose a reason for hiding this comment

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

Valid point
will fix

@PrometheusPi PrometheusPi force-pushed the add_python_lib_to_read_picmi_density branch 2 times, most recently from bda81f3 to 3c72704 Compare October 1, 2025 12:48
@PrometheusPi PrometheusPi force-pushed the add_python_lib_to_read_picmi_density branch from 3c72704 to 9242eaf Compare October 1, 2025 12:51
Copy link
Contributor

@chillenzer chillenzer left a comment

Choose a reason for hiding this comment

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

Offline discussion: The idiomatic approach would probably be to define the corresponding transformations and add them to a call list sympy_parser.parse_expr(s, transformations=transformations). But it is unclear whether we want to build all that infrastructure for some transitional code that will be removed in due time. (Saying this is a sure way for a piece of code to become critical infrastructure relied upon by generations and generations of future users (see https://xkcd.com/2730/).)

Comment on lines 125 to 126
file = open("pypicongpu.json")
sim_dict = json.load(file)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
file = open("pypicongpu.json")
sim_dict = json.load(file)
with open("pypicongpu.json") as file:
sim_dict = json.load(file)

Copy link
Member Author

Choose a reason for hiding this comment

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

Done (already suggested by @ikbuibui)

Comment on lines 111 to 115
while s[0].isspace():
s = s[1:]

while s[-1].isspace():
s = s[:-1]
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
while s[0].isspace():
s = s[1:]
while s[-1].isspace():
s = s[:-1]
s = s.strip()

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks - I forgot about that method :-)

Comment on lines 117 to 118
if s[0] == "(" and s[-1] == ")":
s = s[1:-1]
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this needed only a single time? Otherwise we'd need at least a while loop here.

Suggested change
if s[0] == "(" and s[-1] == ")":
s = s[1:-1]
while s.startswith('('):
if not s.endswith(')'):
raise ValueError(f"Missing closing parenthesis in {s=}.")
s = s[1:-1]

Copy link
Member Author

Choose a reason for hiding this comment

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

I like your code

Comment on lines 101 to 102
s = s.replace("pmacc::math::exp", "exp")
s = s.replace("pmacc::math::pow", "pow")
Copy link
Contributor

Choose a reason for hiding this comment

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

This won't be sufficient. There's a long list of pmacc::math functions that could be generated. It might be more reliable to do s.replace('pmacc::math::', 'std') (or just remove the prefix).

Copy link
Member Author

Choose a reason for hiding this comment

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

That's true - I will s.replace('pmacc::math::', ' ')

Comment on lines +72 to +77
index_first_q = s.find("?")
index_first_c = s.find(":") # this assumes that there are no cases in the first branch

condition = s[0:index_first_q]
case1 = s[index_first_q + 1 : index_first_c]
case2 = s[index_first_c + 1 :]
Copy link
Contributor

Choose a reason for hiding this comment

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

Algorithm for detecting matching parenthesis (adapted for this problem):

from itertools import groupby
from sympy import Piecewise


def nesting_level(c):
    if c == "?":
        return 1
    if c == ":":
        return -1
    return 0


class Counter:
    counter = 0

    def __add__(self, addition):
        self.counter += addition
        return self.counter


def parse_branchless(string):
    # ... do parsing with guaranteed no ternary operators...
    return string


def parse_with_keys(keys_and_expressions):
    if len(keys_and_expressions) == 1:
        return parse_branchless(keys_and_expressions[0][1].strip("?:() "))

    condition = parse_with_keys(keys_and_expressions[:1])
    # poor man's index finding because list.index doesn't support key functions
    else_index = (
        next(
            filter(
                lambda x: x[1][0] == keys_and_expressions[0][0],
                enumerate(keys_and_expressions[1:]),
            )
        )[0]
        + 1
    )
    then_expression = parse_with_keys(keys_and_expressions[1:else_index])
    else_expression = parse_with_keys(keys_and_expressions[else_index:])
    # should be
    # return Piecewise([(then_expression, condition), (else_expression, True)])
    # but I didn't implement the parse function
    return [(then_expression, condition), (else_expression, True)]


def parse(string):
    counter = Counter()
    return parse_with_keys(
        list(
            map(
                lambda x: (x[0], "".join(x[1])),
                groupby(s, lambda c: counter + nesting_level(c)),
            )
        )
    )


if __name__ == "__main__":
    # a little demonstration
    s = "c>1 ? (c<3 ? 5: 6) : (c<-5 ? 42 : 2)"
    print(parse(s))

Copy link
Member Author

Choose a reason for hiding this comment

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

I will add this tomorrow

Copy link
Member Author

@PrometheusPi PrometheusPi Oct 8, 2025

Choose a reason for hiding this comment

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

I might need some more time to do so

PrometheusPi and others added 2 commits October 7, 2025 18:19
Co-authored-by: Tapish Narwal <[email protected]>
Co-authored-by: Julian Lenz <[email protected]>
Comment on lines +122 to +126
with open("pypicongpu.json") as file:
sim_dict = json.load(file)
density_fct_str = sim_dict["species_initmanager"]["operations"]["simple_density"][0]["profile"]["data"][
"function_body"
]
Copy link
Contributor

Choose a reason for hiding this comment

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

Heads-up: Nitpicking coming your way!

I'd probably even unindent that last line. That way the file will only be kept open for as long as it is really necessary. In general, that's a good thing to aim for in order to avoid various risks of file corruption, blocking other code's access, etc. but also to more clearly communicate the dependencies in your code. (json.load(...)'s return value might still hold a reference to the file and its [] operators could potentially rely on that file still being open.)

In practice, the dictionary look-up afterwards is certainly completely irrelevant in that respect but one might choose following such style rules in the harmless and simple cases, so they come naturally in the dangerous and difficult-to-decipher ones as well.

Suggested change
with open("pypicongpu.json") as file:
sim_dict = json.load(file)
density_fct_str = sim_dict["species_initmanager"]["operations"]["simple_density"][0]["profile"]["data"][
"function_body"
]
with open("pypicongpu.json") as file:
sim_dict = json.load(file)
density_fct_str = sim_dict["species_initmanager"]["operations"]["simple_density"][0]["profile"]["data"][
"function_body"
]

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CI:no-compile CI is skipping compile/runtime tests but runs PICMI tests component: tools scripts, python libs and CMake PICMI pypicongpu and picmi related

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants