-
Notifications
You must be signed in to change notification settings - Fork 15
ENH: Adding docstrings to functions in config reader #81
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
base: main
Are you sure you want to change the base?
Changes from 2 commits
52d146f
f3d5607
ad7d0d6
e38fbc5
4b452bd
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -8,22 +8,21 @@ | |
| def parse_config_file(configlocation=None): | ||
| """ | ||
| Reads config data from the config file | ||
|
|
||
| the config file must be called config.yml. | ||
|
|
||
| Parameters | ||
| ---------- | ||
| configlocation : str | ||
| the path on the filesystem to the config file. Optional. If not | ||
| configlocation : (str, optional) | ||
| The path on the filesystem to the config file. If not | ||
| specified pytentiostat looks in the current directory for the | ||
| config files. | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. since i posted another change request, I may as well make this small req too......
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oops holdover from previous version. fixed now |
||
|
|
||
| Returns | ||
| ------- | ||
| config_data : dict | ||
| the configuration data | ||
|
|
||
| """ | ||
|
|
||
| if not configlocation: | ||
| configlocation = "./" | ||
| try: | ||
|
|
@@ -42,6 +41,25 @@ def parse_config_file(configlocation=None): | |
| sys.exit("Directory containing config file, {}, not found. Exiting...".format(configlocation)) | ||
|
|
||
| def get_output_params(config_data, override_ts=None): | ||
| """ | ||
| Obtains output filename and output filepath from config file data | ||
|
|
||
| Parameters | ||
| ---------- | ||
| config_data : dict | ||
| The configuration data | ||
| override_ts : (str, optional) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. parens?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah yeah got it now. |
||
| Unique string to append to output filename string. If not specified will add a time stamp | ||
| hours, minutes, and seconds the experiment was conducted at as a unique identifier. | ||
|
|
||
| Returns | ||
| ------- | ||
| out_name_ts : str | ||
| The output filename with unique identifier. | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. a little unclear, this. maybe a second sentence that discusses how the name is composed rather than just "with unique identifier"?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added what unique identifier is added to the file |
||
| data_out_path : str | ||
| The path to which the output file will be written to. | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah yeah the latter. Now changed |
||
| """ | ||
|
|
||
| data_out_name = config_data["general_parameters"]["data_output_filename"] | ||
| data_out_path = config_data["general_parameters"]["data_output_path"] | ||
| if data_out_path.lower() == "desktop": | ||
|
|
@@ -56,6 +74,24 @@ def get_output_params(config_data, override_ts=None): | |
|
|
||
|
|
||
| def get_lsv_params(config_data): | ||
| """ | ||
| Obtains the parameters for a linear sweep voltammetry experiment | ||
|
|
||
| Parameters | ||
| ---------- | ||
| config_data : dict | ||
| The configuration data | ||
|
|
||
| Returns | ||
| ------- | ||
| start_voltage : float | ||
| The voltage to start the sweep at in volts. | ||
| end_voltage : float | ||
| The voltage to end the sweep at in volts. | ||
| sweep_rate : float | ||
| The rate at which the experiment is performed in millivolts per second. | ||
| """ | ||
|
|
||
| start_voltage = config_data["linear_sweep_voltammetry"]["start_voltage"] | ||
| end_voltage = config_data["linear_sweep_voltammetry"]["end_voltage"] | ||
| sweep_rate = config_data["linear_sweep_voltammetry"]["sweep_rate"] | ||
|
|
@@ -64,13 +100,53 @@ def get_lsv_params(config_data): | |
|
|
||
|
|
||
| def get_ca_params(config_data): | ||
| """ | ||
| Obtains the parameters for a chronoamperometry experiment | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not so sure about these guys, but I think i.e., returns is maybe better than obtains? But I am not so clear on that.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think that phrasing is more clear. I changed it on the other functions that do similar things as well so they would be consistent. |
||
|
|
||
| Parameters | ||
| ---------- | ||
| config_data : dict | ||
| the configuration data | ||
|
|
||
| Returns | ||
| ------- | ||
| voltage : float | ||
| The voltage which will be held at in volts. | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can we find a better way to say it maybe?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah I changed to make more descriptive. |
||
|
|
||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. pls delete line
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. deleted |
||
| time: float | ||
| How long the voltage will be held in seconds. | ||
|
|
||
| """ | ||
|
|
||
| voltage = config_data["chronoamperometry"]["voltage"] | ||
| time = config_data["chronoamperometry"]["time"] | ||
|
|
||
| return voltage, time | ||
|
|
||
|
|
||
| def get_cv_params(config_data): | ||
| """ | ||
| Obtains the parameters for a cyclic voltammetry experiment | ||
|
|
||
| Parameters | ||
| ---------- | ||
| config_data : dict | ||
| the configuration data | ||
|
|
||
| Returns | ||
| ------- | ||
| start_voltage : float | ||
| The voltage to start the experiment at in volts. | ||
| first_turnover : float | ||
| The voltage at which the voltage sweep will first change directions in volts. | ||
| second_turnover: float | ||
| The second voltage at which the voltage sweep will change directions in volts. | ||
| sweep_rate : float | ||
| The rate at which the experiment is performed in mV/s. | ||
| cycle_number: int | ||
| How many times to perform this cycle. | ||
| """ | ||
|
|
||
| start_voltage = config_data["cyclic_voltammetry"]["start_voltage"] | ||
| first_turnover = config_data["cyclic_voltammetry"]["first_turnover_voltage"] | ||
| second_turnover = config_data["cyclic_voltammetry"]["second_turnover_voltage"] | ||
|
|
@@ -81,29 +157,108 @@ def get_cv_params(config_data): | |
|
|
||
|
|
||
| def get_exp_type(config_data): | ||
| """ | ||
| Obtains the type of experiment | ||
|
|
||
| Parameters | ||
| ---------- | ||
| config_data : dict | ||
| The configuration data | ||
|
|
||
| Returns | ||
| ------- | ||
| exp_type : str | ||
| The type of experiment to be run. | ||
| """ | ||
|
|
||
| exp_type = config_data["general_parameters"]["experiment_type"] | ||
|
|
||
| return exp_type | ||
|
|
||
|
|
||
| def get_exp_time(config_data): | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wonder if this function should be called
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That is more descriptive. Now changed |
||
| """ | ||
| Obtains the how long the experiment is to be run for the chronoamperometry case | ||
|
|
||
| Parameters | ||
| ---------- | ||
| config_data : dict | ||
| The configuration data | ||
|
|
||
| Returns | ||
| ------- | ||
| exp_time : float | ||
| How long the experiment will be run in seconds. | ||
| """ | ||
|
|
||
| exp_time = config_data["chronoamperometry"]["time"] | ||
|
|
||
| return exp_time | ||
|
|
||
|
|
||
| def get_rest(config_data): | ||
| """ | ||
| Obtains how long to hold before experiment starts | ||
|
|
||
| Parameters | ||
| ---------- | ||
| config_data : dict | ||
| The configuration data | ||
|
|
||
| Returns | ||
| ------- | ||
| rest_time : float | ||
| How long the starting voltage will be held in seconds. | ||
|
|
||
| """ | ||
|
|
||
| rest_time = config_data["general_parameters"]["rest_time"] | ||
|
|
||
| return rest_time | ||
|
|
||
| def get_steps(config_data): | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I still don't quite get why we are having functions that just retrieve dictionary values..... I guess it can make sense if it is part of our user-interface, and we don't expect our users to know python very well. Is that the case?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Though I expect users won't know python well, this part won't really be front-end. I envision in the end, user will enter into visual boxes which, when starting experiment, will write a config (which can be loaded if desired to conduct the same experiment later). I just thought compartmentalizing the experimental config reading here might be good so these functions can read in and then go through a series of checks to make sure they are appropriate values (I still don't have these checks in for most of these functions) instead of bundling that with the functions of the other modules. I can just remake it and place the reading and the checks in the other modules where they are used though if that makes more sense.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see. Actually, this is part of the MVC pattern and it would be the controller layer that does the validation, but it would probably be structured differently with a few validation functions that are more abstract (think about validating file paths, emails or passwords), but here we would be validating that floats are within some range or sthg and then return true or false and it would have statements like We can certainly do it your way, but we are writing a lot of code and a way that does it robustly with less code is probably preferred. Maybe we can discuss this a bit more on the call and make a decision. it will also depend on the UC a bit, so spending time hardening this code (docstrings and tests) may not be the best just yet (though I am glad this is now your impulse and wanting the code to validate!). I think it has also been helpful for learning. I am very excited about how you are now thinking about the coding!
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Okay yeah this makes sense, let's discuss this in the next meeting. Especially because we were going to discuss MVC anyways! |
||
| """ | ||
| Obtains how many points will be measured during experiment | ||
|
|
||
| Parameters | ||
| ---------- | ||
| config_data : dict | ||
| The configuration data | ||
|
|
||
| Returns | ||
| ------- | ||
| step_number : int | ||
| How many points will be measured during the experiment | ||
| """ | ||
|
|
||
| step_number = config_data["general_parameters"]["step_number"] | ||
|
|
||
| return step_number | ||
|
|
||
|
|
||
| def get_adv_params(adv_config_data): | ||
| """ | ||
| Obtains parameters that shouldn't be altered by beginning users largely related to calibration. | ||
|
|
||
| Parameters | ||
| ---------- | ||
| config_data : dict | ||
| The configuration data | ||
|
|
||
| Returns | ||
| ------- | ||
| conversion_factor : float | ||
| Value to convert output number to real voltage measured. | ||
| setpoint_adjuster: float | ||
| Value to shift the starting scheduled voltage to match the input voltage. | ||
| shunt_resistor: float | ||
| Value to adjust for internal resistance largely set by shunt resistor to measure current passed. | ||
| time_step: float | ||
| Minimum time to execute read/write cycle. | ||
| average_number: int | ||
| Number of times the measurement at each point will be performed. | ||
| """ | ||
|
|
||
| conversion_factor = adv_config_data["advanced_parameters"]["conversion_factor"] | ||
| set_gain = adv_config_data["advanced_parameters"]["setpoint_gain"] | ||
| set_offset = adv_config_data["advanced_parameters"]["setpoint_offset"] | ||
|
|
@@ -119,6 +274,7 @@ def get_adv_params(adv_config_data): | |
| time_step, | ||
| average_number, | ||
| ) | ||
|
|
||
| def check_config_inputs(arg): | ||
| """ | ||
| Checks that all the data that should be numerical from that config | ||
|
|
@@ -133,8 +289,8 @@ def check_config_inputs(arg): | |
| _______ | ||
| is_number: Boolean | ||
| Value is True if the arg is a number, False if not. | ||
|
|
||
| """ | ||
|
|
||
| try: | ||
| return isinstance(float(arg), float) | ||
| except: | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line should be put back in. First line is a one-line (<80 chars) description of the overall purpose of the function, then a blank line, then as many lines as you want to expand on the explanation, then a blank line, then Parameters underlined and so on.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, makes sense!