pasqal-io / qadence

Digital-analog quantum programming interface
https://pasqal-io.github.io/qadence/latest/
Apache License 2.0
72 stars 21 forks source link

[Feature] Add backend, diff mode etc to config constructor #490

Closed smitchaudhary closed 4 months ago

smitchaudhary commented 4 months ago
dominikandreasseitz commented 4 months ago

hey @smitchaudhary , @Roland-djee will take over the review here !

smitchaudhary commented 4 months ago

@Roland-djee

I'd be inclined to default it to 0 features, check for it and skip adding an empty block.

Though forcing a default behavior for the fm_config, and thus, moving it after observable_config would be potentially confusing to the user I am afraid.

Or do you mean, make it a non-optional argument, thus preserving the FM->ansatz->Observable order, and force the user to provide a FeatureMapConfig object (which could then default to 0 features), even in cases where they don't want a FM?

RolandMacDoland commented 4 months ago

@Roland-djee

I'd be inclined to default it to 0 features, check for it and skip adding an empty block.

Though forcing a default behavior for the fm_config, and thus, moving it after observable_config would be potentially confusing to the user I am afraid.

Or do you mean, make it a non-optional argument, thus preserving the FM->ansatz->Observable order, and force the user to provide a FeatureMapConfig object (which could then default to 0 features), even in cases where they don't want a FM?

Yes, that's the solution I see. Then we can check the number of features and skip the empty block in the full_fm creation.

RolandMacDoland commented 4 months ago

@Roland-djee

I'd be inclined to default it to 0 features, check for it and skip adding an empty block.

Though forcing a default behavior for the fm_config, and thus, moving it after observable_config would be potentially confusing to the user I am afraid. Or do you mean, make it a non-optional argument, thus preserving the FM->ansatz->Observable order, and force the user to provide a FeatureMapConfig object (which could then default to 0 features), even in cases where they don't want a FM?

Yes, that's the solution I see. Then we can check the number of features and skip the empty block in the full_fm creation.

Maybe a suggestion would be to use a datastructure for handling configs ? Could be an overkill though.

smitchaudhary commented 4 months ago

Thanks @awennersteen and @Roland-djee for the comments.

I have made the following changes:

  1. FeatureMapConfig now defaults to num_features=0 and if a user does not want an FM, they can pass FeatureMapConfig. The downside being that a user who wants to have a single feature, will actively have to specify num_features=1 (which is good practice already :P )
  2. The default behavior of the AnsatzConfig is to have depth=1 (all other ansatz constructors have this default, thus made the most sense).
  3. Removed the ad-hoc block that changed the initial state in case of RYDBERG strategy being used. This was there due to the issue I raised in qadence-libs here. But I think, it is a lot more transparent to have the user specify the state when calling forward.
  4. Modified tests to account for this changes.
  5. Some other stylistic changes + fixing typos.
RolandMacDoland commented 4 months ago

@smitchaudhary Feel free to request a review.

smitchaudhary commented 4 months ago

Re-requested his review for now.

smitchaudhary commented 4 months ago

I will merge it today and bump the version to 1.7.1 with all the changes made in the last 3 weeks.