Skip to content

Improve docstring for iotools.tmy.read_tmy3 and read_tmy2 #714

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

Closed
cwhanse opened this issue May 7, 2019 · 7 comments
Closed

Improve docstring for iotools.tmy.read_tmy3 and read_tmy2 #714

cwhanse opened this issue May 7, 2019 · 7 comments

Comments

@cwhanse
Copy link
Member

cwhanse commented May 7, 2019

Docstrings are text copied from the original MATLAB functions. Issues include:

  1. inconsistent with the pvlib variable naming convention, e.g. GHI instead of ghi
  2. not formatted to render properly in sphinx
  3. inconsistent with actual function (columns names in returned data are not what is described in the table).

Additional issues to address:

  1. parameter descriptions describe output with a hour-ending timestamp convention, but functions return data with hour-beginning timestamps
  2. improve consistency in returned parameters among the read_tmy2, read_tmy3 and epw functions. Discussion here
@AdamRJensen
Copy link
Member

Hi @cwhanse - do you know how many of these issues are still relevant (besides the variable mapping which I know is lacking)?

@cwhanse
Copy link
Member Author

cwhanse commented Aug 1, 2022

@AdamRJensen I wish I'd listed specifics when I opened this issue. I think 2, probably 3, and 4 are fixed in #1494. 5 is similar to 1, in that iotools.read_tmy3 returns with column names consistent with TMY3 documentation, but not with pvlib internal variables. In contrast, iotools.read_epw uses pvlib-python names where the columns are a pvlib quantity.

I am not advocating for read_tmy3 to return pvlib-python names; keeping TMY3 names helps with debugging, maintenance, etc. Maybe a tmy3_to_pvlib function that renames or copies certain columns would be appreciated.

@AdamRJensen
Copy link
Member

The read_tmy3 function certainly shouldn't only return pvlib variable names, but I suppose it is desirable to have the map_variables options as many of the other iotools? Ideally, I think all the iotools functions should have map_variables=True by default.

@cwhanse
Copy link
Member Author

cwhanse commented Aug 2, 2022

Oh yes I had forgotten about the map_variables capability. I agree, it would be good to add to read_tmy3. In a new PR, though.

I wonder what use is being made of the current rename_columns. The new names are more readable. Are they also internal names for some other software?

@wholmgren
Copy link
Member

Are they also internal names for some other software?

as you may know, they match MATLAB PVLIB's tmy reader. I'm not aware of others.

@AdamRJensen
Copy link
Member

I would suggest we deprecate the recolumn argument (which removes the units from the column names) and instead add map_variables (which would map the original column names to pvlib names). Thoughts?

recolumn (bool, default True) – If True, apply standard names to 
TMY3 columns. Typically this results in stripping the units from the column name.

The description also seems misleading, as recolumn does not apply the pvlib "standard names"..

@AdamRJensen
Copy link
Member

@cwhanse @wholmgren Can I go ahead and close this issue and open a new issue/PR concerning the deprecation of rename_columns and addition of map_variables to the read_tmy3 function?

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

No branches or pull requests

3 participants