Skip to content

Support pathlib.Path in Python API #1535

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
nikosavola opened this issue Nov 17, 2023 · 7 comments · Fixed by #1545
Closed

Support pathlib.Path in Python API #1535

nikosavola opened this issue Nov 17, 2023 · 7 comments · Fixed by #1545
Milestone

Comments

@nikosavola
Copy link
Contributor

nikosavola commented Nov 17, 2023

Hi, I always forget to cast Paths to strings while using the Python API. For example

from pathlib import Path
import klayout.db as db

some_path = Path.cwd().parent

...
l2n = db.LayoutToNetlist(SOME_CELL.begin_shapes_rec(0))
l2n.extract_netlist()
l2n.netlist().write(some_path / "netlist.spice", ...)

will fail as some_path / "netlist.spice" is not actually supported and needs to be converted to a string . I was wondering would there be a simple way to do this conversion in the KLayout API side before the underlying C++ calls instead?

@klayoutmatthias
Copy link
Collaborator

klayoutmatthias commented Nov 17, 2023

If pathlib supports str(path) the option is to extend python2c_func<std::string>::operator() in pyaConvert.cc:171 to do the equivalent of str(...) using PyObject_Str.

It's worth a try, although you need to consider different and specifically old Python versions down to 2.7 which is still in use.

@sebastian-goeldi
Copy link
Contributor

It does in fact support casting to string. Afaik since it was added, it has always supported __str__(). pathlib was new in 3.4 though if that causes issues, I would rather rely on throwing an error message. Afaik this error message is way more readable than if there are casting issues due to multiple constructors being available in C++ ;)

@klayoutmatthias
Copy link
Collaborator

Problem is probably that converting to str(..) is a catchall case and this will hide overloads with a different type. So the string conversion needs to be second priority and there is no API support for this case yet.

That's the price for binding C++ concepts to a dynamic language :(

@sebastian-goeldi
Copy link
Contributor

Hmm, I can't see a downside in this case to do a conversion to string. Any object that would be passed to a parameter being used as a path should be or at least expect to be casted to a string (the low level function in python to handle paths are all string based). So even if you were to stringify a numpy array or something, I think the error should be readable?

@klayoutmatthias
Copy link
Collaborator

That true, but there is a overload selection pass first. So let's say there is a method with two overloads:

a.method(int)   # e.g. a.method(1)
a.method(str)   # e.g. a.method("1")

implicitly doing a str on the argument would make an integer match both ways and create an overload conflict. So the overload selection needs to first check in strict mode and less strict in a second pass. This is there already in a way, but not for strings. That is not rocket science, but that means the implementation is not a one-liner change and some care needs to be taken.

@klayoutmatthias
Copy link
Collaborator

Regarding the numpy approach: I have not really considered that case, but that is an interesting option. Referring to #1530 I'm not sure about the performance penalty of the string back and forth conversion, but at least the loops are executed in C++ space then.

I'm always surprised how much is still missing ... I though the API is pretty complete :(

@sebastian-goeldi
Copy link
Contributor

Ah, sorry, I think I mixed up stuff a bit.

I was purely referring to load/read/write cases that expect a string. All of the one I have used, use path as the first parameter in all function footprints. In that case I would argue the conversion is safe, or is it not?

Regarding numpy, I can reply directly in that issue.

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

Successfully merging a pull request may close this issue.

3 participants