qiskit-community / qiskit-nature

Qiskit Nature is an open-source, quantum computing, framework for solving quantum mechanical natural science problems.
https://qiskit-community.github.io/qiskit-nature/
Apache License 2.0
304 stars 206 forks source link

Bopes Sampler tutorial has incorrect if tests and output has warnings #247

Closed woodsp-ibm closed 3 years ago

woodsp-ibm commented 3 years ago

docs/tutorials/05_Sampling_potential_energy_surfaces.ipynb

line [9]

<>:6: SyntaxWarning: "is not" with a literal. Did you mean "!="?
<>:6: SyntaxWarning: "is not" with a literal. Did you mean "!="?
<ipython-input-9-a52335c0e03d>:6: SyntaxWarning: "is not" with a literal. Did you mean "!="?
  if condition is not 'np':

and similar in ln[12]

<>:6: SyntaxWarning: "is not" with a literal. Did you mean "!="?
<>:6: SyntaxWarning: "is not" with a literal. Did you mean "!="?
<ipython-input-12-aea40004e9fe>:6: SyntaxWarning: "is not" with a literal. Did you mean "!="?
  if condition is not 'np':
woodsp-ibm commented 3 years ago

In the latest code it is printing this too

The measured number of particles 1.998919964515065 does NOT match the expected number of particles 2!
The measured number of particles 2.1923056239385375 does NOT match the expected number of particles 2!
The measured number of particles 2.1306926534684445 does NOT match the expected number of particles 2!
The measured number of particles 2.331470449507494 does NOT match the expected number of particles 2!
The measured number of particles 1.9536592471687682 does NOT match the expected number of particles 2!
The measured number of particles 0.8274093631108661 does NOT match the expected number of particles 2!
The measured number of particles 2.080520198403325 does NOT match the expected number of particles 2!
The measured number of particles 1.7168938121529553 does NOT match the expected number of particles 2!
The measured number of particles 1.9209461627090578 does NOT match the expected number of particles 2!
...

which the currently published notebook on stable branch does not do (@mrossinek in case you have any thoughts - in any case I'll look after taking care of issue #242 otherwise that warning ends up in output in the notebook too)

Update: I see its coming from here https://github.com/Qiskit/qiskit-nature/blob/4f1e53f274d3456c550101344db3618647ebae79/qiskit_nature/properties/second_quantization/electronic/particle_number.py#L175-L177 when interpreting the result against the expected. Certainly the state that is arrived at above is not so great in that regard. I added a unit test to cover more of BopesSampler (in particular the code that is particular to VQE as that was not at all tested). And I see the same sort of warning - here is a sample result of the properties for H2.

0: # Particles: 2.295 S: 0.674 S^2: 1.128 M: 0.149

The spin is off as well yet I see there are no complaints in that regard - (I see there is a TODO in the angular momentum property about having a reference spin)

FYI: Particle warnings like above also appear in the thermodynamics tutorial.

mrossinek commented 3 years ago

Thanks for catching this! I will make the relative and absolute tolerances of that check configurable as part of #303 However, those numbers are really far off sometimes. I think what one would have to do is disable that logger:

logging.getLogger("qiskit_nature.properties.second_quantization.electronic.particle_number").setLevel(logging.ERROR)
mrossinek commented 3 years ago

After talking to @pbark offline, we decided to move this logging call to info which should eliminate the problem of unwanted warnings in the tutorials. The fact that this will be logged is documented which should help users who are interested in this information find it :+1:

woodsp-ibm commented 3 years ago

We can also think about how to do things like this in regards to the sort of validation of the experiment that has been discussed recently - i.e. around validating input values/configuration etc for the user to better assure their computation is likely to succeed. Here its validating the result against expected values - it's the reason these aux ops were added of course to allow an end user to see these other properties and make a judgement on their result. Moving it to info logging is fine. I had wondered about some more global flag that would enable such validation checks as this - but I think that's best left until validation is looked at more comprehensively.

I will disable the warning locally when fixing the tutorial so the messages do not get emitted then for the notebook that will be checked in as I imagine I will do that later today ahead of 303 being ready.

mrossinek commented 3 years ago

I absolutely agree! I do want to go back to that discussion of user input validation and improving output awareness. I also like the idea of some sort of a global switch to turn such checks on/off.

But like you say, let's leave this until we revisit that discussion :+1: