Field Vectors attributes should only be floats

[GeneralSarsby]

The doc string says that the parameters can only be floats, however this goes unchecked and silently ignored.
This causes bugs far upstream.

Proposed changes: make the parameters floats.

There are two entry points for a parameter: inside set_component and during init.
The rest of _compute_unknowns, set_vector, _set_attribute_values, and _set_attribute_value, despite what the names would suggests do not need changing.

Backstory:
The AMI430 magnet uses a field vector inside the 3-axis driver.
This driver sets the the 'r' component of the vector to np.int32(0). This is fine, as numpy casting means
most things work out.
Then, the snapshot of the driver is supposed to return a json serializable object, including the unchecked 'r' value however numpy.in32 s are not json serializable.
This means snapshot of the driver fails, crashing user defined acquisition code.

Weather the AMI430 driver /should/ set 'r' to np.int32(0) is a different bug and pull request.

Regards,

Matt

Simple reproducible bug:

    a = FieldVector(r=np.int32(0),theta=0,phi=0)
    json.dumps(a.get_components('r','theta','phi'))

[astafan8]

Are you using the latest qcodes? I'm asking because the issue of JSON serializing a station snapshot with numpy (and other) type has been resolved, see this line where a specific JSON serializer class is being passed to json.dumps.

We should strive to fix the right things. If you're using the latest qcodes, then the bug should be somewhere else (not in the field vector). Otherwise, I'd love to hear more on "Whether the AMI430 driver /should/ set 'r' to np.int32(0) is a bug" :)

[astafan8]

( Regardless of the comment above, (almost all) the changes in this PR are valid )

[GeneralSarsby]

Are you using the latest qcodes? I'm asking because the issue of JSON serializing a station snapshot with numpy (and other) type has been resolved, see this line where a specific JSON serializer class is being passed to json.dumps.

We are using the latest qcodes. However we are using the standard json encoder without the qcodes extension ( I didn't know it was needed/existed). Sometimes using fast-json if performance is needed.
We have tooling to view the json based metadata in a database that runs outside of qcodes.dataset.measurements.py

We had assumed that the snapshot should be JSON-encodable because the docstring for qcodes.instrument says that the snapshot dictionary is the "State of the instrument as a JSON-compatible dict." which I had believed. However if it always needs to go through the pre-processor described in
NumpyJSONEncoder from qcodes.utils.helpers then that should be expressed in the documentation, or abstracted away so that it is true. It could be that NumpyJSONEncoder should be called during instrument.shapshot so that Qcodes specific extensions to the JSON spec are not needed.

We should strive to fix the right things. If you're using the latest qcodes, then the bug should be somewhere else (not in the field vector). Otherwise, I'd love to hear more on "Weather the AMI430 driver /should/ set 'r' to np.int32(0) is a bug" :)

I fully agree.
There is another bug somewhere outside of Field Vector too.
I believe there is more than one bug that allowed the situation to occur, this pull request is only to fix one of them (trying to fix one thing at once).
During my chase to find the source of the bug I discovered that would have been squashed if Field Vector was implemented as documented. However the AMI430 setting r to np.int32(0) is another bug that I have not yet tracked down yet. If I find it I plan to submit a new pull request.

( Regardless of the comment above, (almost all) the changes in this PR are valid )

If I can fix the 'almost' let me know.

Cheers,
Matt.

[WilliamHPNielsen]

@GeneralSarsby, I think the "almost" part is basically just some missing parentheses that make the tests fail.

[astafan8]

We are using the latest qcodes. However we are using the standard json encoder without the qcodes extension ( I didn't know it was needed/existed). Sometimes using fast-json if performance is needed.
We have tooling to view the json based metadata in a database that runs outside of qcodes.dataset.measurements.py

This tells me that you are not using the "qcodes infrastructure" for storing snapshots, right? in this case, you seem to be doing something hacky/unsupported.

Just to explain the custom encoder - the python default one has a limited support for data types, for example not all funky numpy stuff is supported. For the reason of supporting more datatypes, qcodes uses an custom extended json encoder class. Performance is fine, but there is no surprise that some performance penalties may be expected when non-standard python types are used.

... so that Qcodes specific extensions to the JSON spec are not needed

As i already explain above, this is unavoidable because not everything useful is built into python, and not everything should.

However if it always needs to go through the pre-processor described in
NumpyJSONEncoder from qcodes.utils.helpers then that should be expressed in the documentation, or abstracted away so that it is true. It could be that NumpyJSONEncoder should be called during instrument.shapshot so that Qcodes specific extensions to the JSON spec are not needed.

here we need to be careful. "snapshot" is initially a python dictionary, and that's what snapshots are until they arrive to the Measurement class of qcodes. Because the snapshot needs to be saved before the measurement run, its python dict needs to be converted to something language-agnostic which was chosen to be JSON (some time ago). This shows that snapshots at the level of instruments do not know about how they will be stored, which brings the general flexibility of using other formats like pickle, xml, whatever.

I believe there is more than one bug that allowed the situation to occur, this pull request is only to fix one of them (trying to fix one thing at once).

I'd rather have a full picture of what is going on because "fixing one thing at once" may change the other bug that you happen to be looking into.

To conclude: let know about the other bug(s) and then it will be clear what fixes need to be done.

[codecov]

Codecov Report

Merging #1458 into master will increase coverage by 0.03%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #1458      +/-   ##
==========================================
+ Coverage   73.78%   73.81%   +0.03%     
==========================================
  Files          92       92              
  Lines       10439    10435       -4     
==========================================
+ Hits         7702     7703       +1     
+ Misses       2737     2732       -5

[astafan8]

After an offline discussion with @GeneralSarsby , we've agreed to proceed with this PR so that there is no ambiguitiy in types of values insidethe field vector. The driver-related bugs will be discovered and tackled in other PRs.

@QCoDeS/core could you review this and see if you agree with this suggested change? (i'm gonna add an explicit test for the float conversion soon)

[jenshnielsen]

Apart from the type checking issue in CI and the missing tests this looks fine