[CQT-414] mip mapper maps circuits that do not need to be remapped

[rares1609]

No description provided.

[elenbaasc]

Thanks for the effort! It seems it was a bit more work that I had anticipated :)

Mostly minor comments. Mainly in regards to the implementation of the map method where you default to using numpy arrays, when simple objects would suffice or are expected in the end anyway, e.g. creating a numpy array and at the end converting it to a list.

[elenbaasc]

This is a static method:

Suggested change

	def _get_reference_counter(self, ir: IR, num_virtual_qubits: int) -> list[list[int]]:
	@staticmethod
	def _get_reference_counter(ir: IR, num_virtual_qubits: int) -> list[list[int]]:

[elenbaasc]

This is still not a static method.

[rares1609]

Sorry about that, could have sworn I made it static before commiting 😢

[elenbaasc]

Is this still needed, now that you updated the Mapping.__eq__ method? Maybe this condition self.data == other.data should not be part of that method.

[rares1609]

Indeed this is no longer needed, so i removed the .items() from the assertions, in both test_simple_mappers.py and test_mip_mapper.py. Regarding the condition in Mapping.__eq__ what should be done about it? Should this be part of a different issue?

[elenbaasc]

Good :)

Removing the following lines from the Mapping.__eq__:

if self.data != other.data:
            return False

should do the trick. (Fingers crossed that the tests succeed...)

[rares1609]

Thankfully they work 😆

[elenbaasc]

I see that you removed:

for key in self.data:
    if self.data[key] != other.data[key]:
        return False

but that the following is still there:

return self.data == other.data

The first one should be there, i.e., you are checking if all values match. The second one should not be there, because you don't want to say that the object self.data must be equal to the object other.data, for one mapping to be equal to another.

[rares1609]

I replaced the statement checking if all values match with return all(self.data[key] == other.data[key] for key in self.data).

[elenbaasc]

Thanks for the effort! Almost there :)

Some comments not fully resolved yet and a small comments about the placement of the docstring.

[elenbaasc]

Shouldn't these docstring be part of the map method (as it is the only public method of this class)? It might even be the docstring of the class... docstrings in private classes are not published in the user docs, and are only there for the developers.

[elenbaasc]

I see that you removed:

for key in self.data:
    if self.data[key] != other.data[key]:
        return False

but that the following is still there:

return self.data == other.data

The first one should be there, i.e., you are checking if all values match. The second one should not be there, because you don't want to say that the object self.data must be equal to the object other.data, for one mapping to be equal to another.