ENH: Adding docstrings to functions in config reader by aplymill7 · Pull Request #81 · juami/pytentiostat

aplymill7 · 2019-09-18T15:00:34Z

No description provided.

codecov-io · 2019-09-18T15:03:02Z

Codecov Report

Merging #81 into master will not change coverage.
The diff coverage is 100%.

@@           Coverage Diff           @@
##           master      #81   +/-   ##
=======================================
  Coverage   70.81%   70.81%           
=======================================
  Files           7        7           
  Lines         209      209           
=======================================
  Hits          148      148           
  Misses         61       61

Impacted Files	Coverage Δ
pytentiostat/config_reader.py	`52.63% <100%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ecdb2de...4b452bd. Read the comment docs.

sbillinge

excellent!

sbillinge · 2019-09-21T11:00:34Z

+    config_data : dict
+        the configuration data
+
+    override_ts : Unique string to add after output filename in stringe. Optional.


slightly wrong structure to this one. type is string and its optional on this line, the description of what id does on the next line down.

Also, as per the other comments, remove empty lines between parameters and return values. Also, good time to fix small typo in here.

So I tried override_ts: (str, optional), if that seems fine? I saw it on an example doc string. Or maybe should just be str, optional?

I prefer the latter:
override_ts: str, optional

Let's set the style to this maybe?

Sounds good!

sbillinge · 2019-09-21T11:01:35Z

+    Parameters
+    ----------
+    config_data : dict
+        the configuration data


capitalize the here, remove mt lines, but this is super!

Removing mt lines, capitalizing in descriptions, fixing typos.

sbillinge

very nice, nearly there!

sbillinge · 2019-09-22T21:37:18Z

 def parse_config_file(configlocation=None):
    """
    Reads config data from the config file
-


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.

Okay, makes sense!

sbillinge · 2019-09-22T21:43:35Z


 def get_ca_params(config_data):
+    """
+    Obtains the parameters for a chronoamperometry experiment


I'm not so sure about these guys, but I think Returns the parameters for a chronoamperometry experiment from the config data

i.e., returns is maybe better than obtains? But I am not so clear on that.

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.

Changing obtain to returns in all config data retriever functions, readding blank in first function docstring, and removing parens on str, optional

sbillinge

few more comments....got a little deeper into the functions this time....

sbillinge · 2019-09-22T22:56:47Z

+    ----------
+    config_data : dict
+        The configuration data
+    override_ts : (str, optional)


Ah yeah got it now.

sbillinge · 2019-09-22T22:57:59Z

+    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.


since i posted another change request, I may as well make this small req too......config file (singular) now?

Oops holdover from previous version. fixed now

sbillinge · 2019-09-22T22:59:23Z

+    out_name_ts : str
+        The output filename with unique identifier.    
+    data_out_path : str
+        The path to which the output file will be written to.


The path where the file will be written?
or
The path to the directory where the file will be written?

Ah yeah the latter. Now changed

sbillinge · 2019-09-22T23:00:10Z

+    Returns
+    -------
+    out_name_ts : str
+        The output filename with unique identifier.    


a little unclear, this. maybe a second sentence that discusses how the name is composed rather than just "with unique identifier"?

Added what unique identifier is added to the file

sbillinge · 2019-09-22T23:00:45Z

+    -------
+    voltage : float
+        The voltage which will be held at in volts.
+


pls delete line

sbillinge · 2019-09-22T23:01:53Z

+    Returns
+    -------
+    voltage : float
+        The voltage which will be held at in volts.


can we find a better way to say it maybe?

Yeah I changed to make more descriptive.

sbillinge · 2019-09-22T23:03:47Z

    return exp_type


 def get_exp_time(config_data):


I wonder if this function should be called get_experiment_duration?

That is more descriptive. Now changed

sbillinge · 2019-09-22T23:04:39Z


 def get_exp_time(config_data):
+    """
+    Returns how long the experiment is to be run for the chronoamperometry case.


why isn't this info returned with the chronoamperometry info? Wouldn't that be better maybe?

I just wrote this for plotting module since the only data it needs for the CA experiment is the time (not the voltage). But maybe this one is separate enough that I should just read from the dictionary in plotter and get rid of this function. Check for it could already be made by get CA params.

sbillinge · 2019-09-22T23:07:10Z


    return rest_time

 def get_steps(config_data):


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?

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.

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

if validate_voltage(config_data["ca"]["initial_voltage"], -5., 5.): ca_initial voltage = config_data["ca"]["initial_voltage"]

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!

Okay yeah this makes sense, let's discuss this in the next meeting. Especially because we were going to discuss MVC anyways!

ENH:Adding docstrings to functions in config reader

52d146f

sbillinge reviewed Sep 21, 2019

View reviewed changes

STYL: out_params doc strings

f3d5607

Removing mt lines, capitalizing in descriptions, fixing typos.

sbillinge reviewed Sep 22, 2019

View reviewed changes

STYL: Improving the docstrings.

ad7d0d6

Changing obtain to returns in all config data retriever functions, readding blank in first function docstring, and removing parens on str, optional

sbillinge reviewed Sep 22, 2019

View reviewed changes

aplymill7 added 2 commits September 22, 2019 18:52

STYL: making docstring changes

e38fbc5

ENH: changing plotter from time -> duration

4b452bd

aplymill7 added the documentation Explains how the code works or how to use it label Oct 15, 2019

Uh oh!

Conversation

aplymill7 commented Sep 18, 2019

Uh oh!

codecov-io commented Sep 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

sbillinge left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sbillinge left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sbillinge left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

codecov-io commented Sep 18, 2019 •

edited

Loading