Skip to content
Open
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 25 additions & 11 deletions pulser-core/pulser/register/base_register.py
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,8 @@ def __init__(
if not isinstance(qubits, dict):
raise TypeError(
"The qubits have to be stored in a dictionary "
"matching qubit ids to position coordinates."
"matching qubit ids to position coordinates; "
f"got {type(qubits)}."
)
if not qubits:
raise ValueError(
Expand Down Expand Up @@ -98,7 +99,7 @@ def _init_kwargs(self, **kwargs: Any) -> None:
if kwargs.keys() != {"layout", "trap_ids"}:
raise ValueError(
"If specifying 'kwargs', they must only be 'layout' and "
"'trap_ids'."
f"'trap_ids'; got {sorted(kwargs)}."

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I am not sure the sorted is needed here. Since the target of this message is a software developer, I would rather return the kwargs in the order the program submitted them.

)
layout: RegisterLayout = kwargs["layout"]
trap_ids: tuple[int, ...] = tuple(kwargs["trap_ids"])
Expand Down Expand Up @@ -149,7 +150,9 @@ def find_indices(self, id_list: abcSequence[QubitId]) -> list[int]:
if not set(id_list) <= set(self.qubit_ids):
raise ValueError(
"The IDs list must be selected among the IDs of the register's"
" qubits."
" qubits; "
f"{sorted(set(id_list) - set(self.qubit_ids))} not in "

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I don't think the sorted is necessary here, and I would suggest to take it out as it might take unnecessary time to raise this error.

f"{list(self.qubit_ids)}."
)
return [self.qubit_ids.index(id_) for id_ in id_list]

Expand Down Expand Up @@ -190,14 +193,15 @@ def from_coordinates(
if labels is not None:
raise NotImplementedError(
"It is impossible to specify a prefix and "
"a set of labels at the same time"
"a set of labels at the same time; "
f"got prefix={prefix!r} and labels={list(labels)}."
)

elif labels is not None:
if len(coords_) != len(labels):
raise ValueError(
f"Label length ({len(labels)}) does not"
f"match number of coordinates ({len(coords_)})"
f"Label length ({len(labels)}) does not "
f"match number of coordinates ({len(coords_)})."

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Maybe report the two offending inputs: "Got coords {coords} and labels {labels}.".

)
qubits = dict(zip(cast(Iterable, labels), coords_))
else:
Expand All @@ -212,15 +216,21 @@ def _validate_layout(
if register_layout.dimensionality != self.dimensionality:
raise ValueError(
"The RegisterLayout dimensionality is not the same as this "
"register's."
f"register's; layout is {register_layout.dimensionality}D "
f"and register is {self.dimensionality}D."
Comment on lines 217 to +220

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I think what matters is knowing the input layout
. The information you are bringing has value, you can modify the error raised:
"The RegisterLayout dimensionality ({register_layout.dimensionality}D) is not the same as this "
f"register's ({self.dimensionality}D); Got layout {register_layout} on register {self.register}."

)
if len(set(trap_ids)) != len(trap_ids):
raise ValueError("Every 'trap_id' must be a unique integer.")
repeated = sorted({t for t in trap_ids if trap_ids.count(t) > 1})

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I think it would be best to try to return the repeating elements in their order of appearance in the trap_ids. Therefore, I suggest to get rid of sorted, and use collections.Counter (offers a faster one-liner than your current solution)

Suggested change
repeated = sorted({t for t in trap_ids if trap_ids.count(t) > 1})
repeated = [t for t, freq in Counter(trap_ids).items() if freq > 1]

raise ValueError(
"Every 'trap_id' must be a unique integer; "
f"found repeated ids {repeated} in {list(trap_ids)}."
)

if len(trap_ids) != len(self._ids):
raise ValueError(
"The amount of 'trap_ids' must be equal to the number of atoms"
" in the register."
f" in the register; got {len(trap_ids)} trap_ids "
f"for {len(self._ids)} atoms."
Comment on lines 231 to +233

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

You can modify the returned message to show the length of trap_ids and self._ids, but what matters is returning the incorrect input:
f"The amount of 'trap_ids' {len(trap_ids)} is not equal to the number of atoms {len(self._ids)}. Got trap ids {trap_ids} for atoms {self._ids}."

)

for reg_coord, trap_id in zip(
Expand All @@ -229,7 +239,9 @@ def _validate_layout(
if np.any(reg_coord != trap_coords[trap_id]):
raise ValueError(
"The chosen traps from the RegisterLayout don't match this"
" register's coordinates."
f" register's coordinates; trap {trap_id} is at "
f"{trap_coords[trap_id].tolist()} but the register "
f"has {reg_coord.tolist()}."
)

def define_detuning_map(
Expand All @@ -251,7 +263,9 @@ def define_detuning_map(
if not set(detuning_weights.keys()) <= set(self.qubit_ids):
raise ValueError(
"The qubit ids linked to detuning weights have to be defined"
" in the register."
" in the register. Got "
f"{sorted(set(detuning_weights) - set(self.qubit_ids))}, "

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Here as well, I don't think the sorted is necessary and suggest to take it of, for the sake of time.

f"which are not in {list(self.qubit_ids)}."
)
return DetuningMap(
pm.vstack(
Expand Down
Loading