Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

QCodes Driver SPECS Zurich #327

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

Conversation

Diddleydope
Copy link

No description provided.

def UserOut_ValSet50(self, Output_value: np.float32):
self.n.UserOut_ValSet(50, Output_value)

def ThreeDSwp_SwpAcqChsSet(self, channelIndexes: np.array(int)):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Here you should probably use NDArray from numpy typing https://numpy.org/devdocs/reference/typing.html#numpy.typing.NDArray (remember to add the import at the top of the file)

You probably want something like NDArray[np.integer] or NDArray[np.int32] if you want a specific integer size

Copy link
Author

Choose a reason for hiding this comment

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

I have changed the things you mentioned. In case you're confused as to why I have "nanonis-tramea-specs" once and "nanonis_tramea_specs" the other time, that's because the actual module name, as in in order to install it, is "nanonis-tramea-specs" and it is imported as "nanonis_tramea_specs". This is bad practice and will be changed soon.

@jenshnielsen
Copy link
Collaborator

@Diddleydope Thanks for the contribution.

There are a few failures in the Automatic CI build.

When we are building the docs we must be able to import the code. Since your code depends on a module that not explicitly installed you should add it here https://github.com/QCoDeS/Qcodes_contrib_drivers/blob/main/docs/conf.py#L127 to tell Sphinx to mock the missing import.

The type checking using mypy fails for a similar reason.

src/qcodes_contrib_drivers/drivers/SPECS/Specs_Tramea.py:4:1: error: Cannot find implementation or library stub for module named "nanonis_tramea_specs" [import-not-found]
src/qcodes_contrib_drivers/drivers/SPECS/Specs_Tramea.py:4:1: note: See https://mypy.readthedocs.io/en/stable/running_mypy.html#missing-imports
src/qcodes_contrib_drivers/drivers/SPECS/Specs_Tramea.py:910:54: error: Invalid type comment or annotation [valid-type]
src/qcodes_contrib_drivers/drivers/SPECS/Specs_Tramea.py:910:54: note: Suggestion: use np.array[...] instead of np.array(...)
src/qcodes_contrib_drivers/drivers/SPECS/Specs_Tramea.py:920:77: error: Invalid type comment or annotation [valid-type]
src/qcodes_contrib_drivers/drivers/SPECS/Specs_Tramea.py:920:77: note: Suggestion: use np.array[...] instead of np.array(...)
src/qcodes_contrib_drivers/drivers/Horiba/Horiba_FHR.py:38:9: note: By default the bodies of untyped functions are not checked, consider using --check-untyped-defs [annotation-unchecked]

You should tell mypy to ignore it here https://github.com/QCoDeS/Qcodes_contrib_drivers/blob/main/pyproject.toml#L76

See comment inline for the numpy issue

Comment on lines 37 to 38
set_cmd=self.UserOut_ValSet1,
get_cmd=self.Signals_ValGet24,

Choose a reason for hiding this comment

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

Can we directly use self.n attributes instead of defining individual functions for each channel?

    set_cmd=partial(self.n.UserOut_ValSet, 1)
    get_cmd=(self.n.Signals_ValGet(24, 1)[2])[0]

To verify, is it intended that get_cmd=self.Signals_ValGet24 (specifically 24) for all OutputX parameters?

Copy link
Author

Choose a reason for hiding this comment

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

I was unaware that you could write partial functions like this in python (this is my first project in python). This is clearly way better. And no, that is not intended, I must have accidentally done that. I will change this immediately. Thank you for the input!

Copy link

codecov bot commented May 23, 2024

Codecov Report

Attention: Patch coverage is 0% with 90 lines in your changes are missing coverage. Please review.

Project coverage is 10.91%. Comparing base (608a88d) to head (3be6e92).
Report is 8 commits behind head on main.

Files Patch % Lines
...odes_contrib_drivers/drivers/SPECS/Specs_Tramea.py 0.00% 90 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #327      +/-   ##
==========================================
- Coverage   10.97%   10.91%   -0.06%     
==========================================
  Files         132      133       +1     
  Lines       17540    17630      +90     
==========================================
  Hits         1925     1925              
- Misses      15615    15705      +90     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants