Skip to content
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

Why were the placers "custom" and "trivial" removed? #1556

Closed
carlos-luque opened this issue Jan 8, 2025 · 9 comments
Closed

Why were the placers "custom" and "trivial" removed? #1556

carlos-luque opened this issue Jan 8, 2025 · 9 comments

Comments

@carlos-luque
Copy link
Contributor

I'm wondering why the "custom" and "trivial" placers were removed in version 2.14. Are there any ways to achieve the same results in the current implementation?

@alecandido
Copy link
Member

Yes, the reason is that you can now just skip the placement step to achieve the same result.

The transpiler object is essentially an instance of Passes (maybe we should even consider changing its name to Transpiler...), which is nothing else than a collection of transformations.
https://qibo.science/qibo/stable/code-examples/advancedexamples.html#how-to-modify-the-transpiler

Each transformation is optional, and it can be applied multiple times. If you want to do nothing (as in Trivial) you just skip the corresponding step. To apply a pre-established placement (as in Custom), you just have to apply that permutation to the .wire_names attribute in your Circuit instance, which is defining the basic association between the abstract qubit names used in the gates (integers in range(n)) and the qubit names in the platform (which may be integers as well, but also strings). Obviously, before any further placement or routing transformation - which may add further swaps to optimize the transpilation result (or even just make execution possible).

@carlos-luque
Copy link
Contributor Author

Thanks so much for your reply and it's crystal clear. I found issue #1500, which defines the new attribute in the 'Circuit' class, along with other changes.

@GuillermoAbadLopez
Copy link
Contributor

GuillermoAbadLopez commented Feb 5, 2025

Hello everybody,
in Qilimanjaro we are also missing the Custom placer now, I was just going to open an Issue for it when I saw this one 😅

Between ourselves we have already discussed how this can be achieved with the wire_names, but some people @fedonman @amirazzam2000 still think that having the possibility of a Placer that changes it for you would be good too.

Imagine, you have a circuit of 5 qubits, and you want to try it on different subgraphs of a chip of 10 qubits, to characterize noise/error of different parts of a chip, etc.. you could do it by looping and manually changing the Circuit.wire_names each time. But that is changing the circuit instance, so if you wanna do that and then make a change in the circuit (adding more gates without having problems in which qubit/wire you're doing so) and then run the circuit again in the same execution for some reason, for example for some characterization of leakage, etc.., we can see it getting complicated at some point.

Instead, if we leave the circuit still, and just change the Custom(Placer) at each execution, it's much easier to be sure you won't have problems changing/updating your circuit since it remains totally unchanged, and you just need to execute the placer each time. Which should be much of an overhead, no?

You might say, well this can course be achieved, just doing a deepcopy of your circuit and changing the wire_names in each loop, and of course you can, but that seems a bit convoluted, we liked and think it is good to have the same interface for passing a Custom placing, than for the rest or Placer's. What do you think, maybe it makes sense to add it back, even though the class itself would be the inherited base Placer class + a single method that changes the wire_names?

(The Trivial one doesn't make sense to us, only asking for the Custom one)

Tagging @alecandido and @carlos-luque since they were original people in the Issue
@csookim since it was who suggested the deletion in the first place in #1500.

@alecandido
Copy link
Member

alecandido commented Feb 5, 2025

You might say, well this can course be achieved, just doing a deepcopy of your circuit and changing the wire_names in each loop, and of course you can, but that seems a bit convoluted, we liked and think it is good to have the same interface for passing a Custom placing, than for the rest or Placer's. What do you think, maybe it makes sense to add it back, even though the class itself would be the inherited base Placer class + a single method that changes the wire_names?

Well, I would argue that building multiple transpilers, all with their own custom placers, is pretty much equivalent to add a step outside the transpiler itself:

# With the custom placer

transpiler1 = ...
transpiler2 = ...

res1 = transpiler1(circuit).execute()
res2 = transpiler2(circuit).execute()

# Without the custom placer

def my_function_which_i_could_name_transpiler_even_without_being_an_instance1(c):
     c = deepcopy(c)
     c.wire_names = ...
     return transpiler(c)

def my_function[...]2(c):
     ...

res1 = my_function[...]1(circuit).execute()
res2 = my_function[...]2(circuit).execute()

and you could even make a single function which is generating them, given the permutation.

I would say that adding another Custom placer does not add much to what you can do and what you can't, since the alternative mechanism is still there.
I know that is quite philosophical at this point, but I'd cite common idioms to avoid reintroducing it

There should be one-- and preferably only one --obvious way to do it.
https://peps.python.org/pep-0020/

@GuillermoAbadLopez
Copy link
Contributor

GuillermoAbadLopez commented Feb 5, 2025

Right, we can totally do that yes, I agree.
But that is why I thought that the strongest point in my previous (maybe took long 😅 ) text , was:

we think it is good to have the same interface for passing a Custom placing, than for the rest or Placer's

But of course, it can be done without it as you showed, and yes the discussion becomes a bit philosophical, without a single better answer, true.. We just think the UI/UX would be a bit better that way, but it's true you are adding a bit of duplication 🤔

@alecandido
Copy link
Member

But of course, it can be done without it as you showed, and yes the discussion becomes a bit philosophical, without a single better answer, true.. We just think the UI/UX would be a bit better that way, but it's true you are adding a bit of duplication 🤔

In some sense, this is something I'm trying to account elsewhere, to make packages as much extensible as possible from users, such that you do not need to affect the distributed package source to support your use case.
This is more or less already the situation for the transpiler, because of the arbitrary steps. However, I acknowledge that the interface is pretty messy, and not very helpful in this sense.

If you feel that it would be rather comfortable to you to have it as a transpiler step, you may just grab the class from a past commit

class Custom(Placer):
"""Define a custom initial qubit mapping.
Args:
map (list or dict): A mapping between physical and logical qubits.
- If **dict**, the keys should be physical qubit names, and the values should be the corresponding logical qubit numbers.
- If **list**, it should contain physical qubit names, arranged in the order of the logical qubits.
"""
def __init__(self, initial_map: Union[list, dict], connectivity: nx.Graph = None):
self.initial_map = initial_map
self.connectivity = connectivity
def __call__(self, circuit: Circuit):
"""Apply the custom placement to the given circuit.
Args:
circuit (:class:`qibo.models.circuit.Circuit`): circuit to be transpiled.
"""
assert_placement(circuit, self.connectivity)
if isinstance(self.initial_map, dict):
circuit.wire_names = sorted(self.initial_map, key=self.initial_map.get)
elif isinstance(self.initial_map, list):
circuit.wire_names = self.initial_map
else:
raise_error(TypeError, "Use dict or list to define mapping.")
assert_placement(circuit, self.connectivity)

and introduce it in your code. Even if it won't be defined as part of the qibo package, there is no reason why it should not work :)

@GuillermoAbadLopez
Copy link
Contributor

just grab the class from a past commit and introduce it in your code.

Yeah, we were considering doing that if you refused, already ;)

(but we have users import all Passes directly from qibo, so having a single different one imported from somewhere else (our software qililab) is a bit ugly... we will think\discuss about it further then)

@alecandido
Copy link
Member

Yeah, we were considering doing that if you refused, already ;)

Smart guys 😏

(but we have users import all Passes directly from qibo, so having a single different one imported from somewhere else (our software qililab) is a bit ugly... we will think\discuss about it further then)

Well, my advice would be to avoid too much focus on what is ugly, since I usually do not have enough time to make things working and solid (no chance to make them beautiful...).

I know that sometimes may be a bit confusing, but you may consider that even NumPy and SciPy are packages supposed to work together, to the point that some linear algebra operations, which you may expect to be NumPy's specialty, are actually part of scipy.linalg.
In this case, the Custom would be just an extension provided for your users' convenience.

As said, I'm just trying to keep things minimal, exactly because I'd like to commit in Qibo on making the interface stable, and focus on features that require deep integration, since it won't be as simple to provide them as extensions...
A larger interface exposes more surface, thus more code to maintain, and more things which can get wrong.
Not that this one would be such a big deal, but that's just to explain the overall rationale (such that you may also argue against it ;P).

In any case, if from qililab sounds little Qibo-ish, you can always try to import from... qibolab!

@GuillermoAbadLopez
Copy link
Contributor

As said, I'm just trying to keep things minimal, exactly because I'd like to commit in Qibo on making the interface stable, and focus on features that require deep integration, since it won't be as simple to provide them as extensions...
A larger interface exposes more surface, thus more code to maintain, and more things which can get wrong.
Not that this one would be such a big deal, but that's just to explain the overall rationale (such that you may also argue against it ;P).

Makes sense, well thank you anyway,
and good luck with it 👌

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

No branches or pull requests

3 participants