Skip to content

Conversation

@FolkLab
Copy link
Contributor

@FolkLab FolkLab commented Jul 6, 2022

Having added a new ipf file named "Scan_n_D_Bxyz"

Copy link
Member

@TimChild TimChild left a comment

Choose a reason for hiding this comment

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

Looks like a lot of good work in here, and already looking quite nice and tidy! I don't think I got a notification when you made this request, so next time send me a message or email or something so I can get to it sooner! There are a few things that should be addressed before merging with the master branch. My comments are below.

If these are scans that others in the lab would find useful, they should be added to the "scans.ipf" file instead of in a separate file.

If they are only useful to you, then they should not be merged with the IgorAcq repo.

Assuming this is useful for others (which it looks like it would be), there are a few changes that would be useful before merging.

  1. Convert functions:
    a. Are the conversion values specific to a system? Or where do they come from? Can you add a comment to describe
    b. Could you add a short description of what each is converting from and to? I don't know what "Vt" or "D" is for example.

  2. Scan functions
    a. Remove any commented code that is not used (e.g. "//// abort ...." and the "do ... while" that is commented out)
    b. Where do the values "Vtgn", "VtgD" etc come from? Can you add a comment
    c. since "y_label" is an optional string, it needs to have a default set with a line like "y_label = selectstring(paramisdefault(y_label), y_label, "R (Ω)")" -- currently that is commented out, and I'm not sure why. You can also have it default to "", but it should not be left without a default at all as strings default to null in Igor and that causes errors.
    d. Add a short description of what each scan does. Maybe this is obvious to 2D people, but it's not obvious to me.
    e. Move the security checks of using 'setK2400Voltage' to a function of its own (that takes the ScanVars S as an argument) to match the style of other scans which include checks before starting. (the aim is to keep the scan functions easily human readable for new Igor users and to prevent duplicate code being written if anyone makes a new scan). Also, I think it should "abort" rather than print and return -1 in the case of bad inputs.

Copy link
Member

@TimChild TimChild left a comment

Choose a reason for hiding this comment

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

good

@TimChild TimChild self-requested a review April 21, 2023 20:39
Copy link
Member

Choose a reason for hiding this comment

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

This change is good

Copy link
Member

Choose a reason for hiding this comment

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

Not looked very closely, but looks good overall.

For now it's ok that this is being added to top level of IgorAcq, but very soon these extra files should be organized somewhere in IgorAcq

Copy link
Member

@TimChild TimChild left a comment

Choose a reason for hiding this comment

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

These changes seem fine to me (not looked closely at the whole new file)

The new file can be added to root folder for now, but very soon needs to be moved to something like an "analysis" o.e. folder in IgorAcq

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.

4 participants