search

tum-pbs / PhiFlow

A differentiable PDE solving framework for machine learning
MIT License
1.39k stars 189 forks source link

Refactor to fix type unions #122

Closed marcelroed closed 1 year ago

marcelroed commented 1 year ago

Python evaluates the expressions in function declarations when the module loads, so

def f(x: Tensor or float):
    pass

evaluates to

def f(x: Tensor):
    pass

since Tensor or float evaluates to float (Python's or operator short circuits and returns the first argument that is truthy). This results in the type hints in PhiFlow being wrong when evaluated by an IDE.

I have contributed some changes to replace type1 or type2 or ... with typing.Union[type1, type2, ...], which is the correct approach. This took way too long, but should improve useability in IDEs with type inference. I will continue to contribute missing types to make the typing system more robust.

From Python 3.10 and onward there is a new way of doing this that looks much cleaner, but for now I think Union is the best approach.

holl- commented 1 year ago

Great! Do you have a tool to do this automatically?

I've changed the target to develop. It will be merged into master upon the next release.

marcelroed commented 1 year ago

Great, thanks! I actually did it half-manually: finding them using ast and solving them with regex + manual checking. I slightly underestimated the scale of the refactor... I think these kinds of replacements could be done quite effectively if there was some automatic code styling applied!

@holl- Would you be positive about using applying linter and codeformatter like black/flake8? I think ruff is a really neat all-in-one and fast implementation maintaining these style guides. I'm happy to contribute it, and we can surely change all the parameters to something that contributors find pleasing. It would autorun on git commit.

holl- commented 1 year ago

Most of the code should already adhere to the guidelines but we explicitly make exceptions with some rules. Is there anything in particular that you would change?

Generally, I think it would be a good idea to add a tool like ruff to auto-run but I don't want it to screw up the formatting we have. This should go into 2.4-develop, though.