Inconsistencies in iotools

[AdamRJensen]

There exists a number of inconsistencies between the functions in pvlib.iotools that I would like to address.

First, the naming of start/stop date differs between functions for unknown reasons. E.g., get_ecmw_macc uses startdate/stopdate, read_midc_raw_data_from_nrel uses start/end and #1175 currently uses start_date/end_date. It would be great with a common consensus of which of the three to use. I'd be willing to change the functions to comply with the decision (of course with deprecation warnings to start with)

Another inconsistency is that meta data is called different things by different functions. Here's a list:

• Meta: pvigs, epw (code), tmy (in code)
• Metadata: epw (in documentation), surfrad, tmy (in documentation)
  and then there's psm3 which calls it "headers".

Again if we can agree on a consensus I'd like to offer to implement the changes.

Lastly, the naming of the two data retrieval functions: read_midc_raw_data_from_nrel and read_srml_month_from_solardat, which ideally should be called get_midc and get_srml to match their read_ counterparts.

[cwhanse]

+1 to normalizing the argument names. I would prefer the simpler start and end and let the docstring speak to the data type (if date and datetime actually are different). For me, metadata > meta but I don't have a strong preference here.

[AdamRJensen]

One thing that also supports start and end is that it can be used for both functions that only require a date as implied by start_date (e.g., 2021-09-06), but also functions that take a full datetime (e.g., 2021-09-06 12:00) which otherwise could be called start_time.

[wholmgren]

I like start, end, and metadata.

While our long and detailed review of #1175 suggests otherwise (sorry), I really have wanted to keep iotools relatively easy to contribute to. Part of that means accepting some inconsistencies, but there's also no reason we can't come through and clean up the API from time to time.

We don't maintain standards for names within actual code other than readability - it's only the API names that matter. And no need to deprecate positional argument names - just function names and keyword arguments.

[AdamRJensen]

@wholmgren @kanderso-nrel All of the get functions return data, metadata with the exception of get_psm3 which returns metadata, data. Are we interested in switching this? If so I can do this now before v0.9.

[kandersolar]

I'm +1 for consistency. Is there a way to somehow deprecate the order of return values? No reasonable solution comes to mind, and some quick googling didn't turn up anything either. Maybe this should just be a sudden breaking change, with the excuse that pvlib is still pre-1.0.

[wholmgren]

I'm fine with a breaking change or with leaving it alone for now. Continued growth of pvlib could support that it's better to make current users go through a little pain for the betterment of the larger pool of future users. At our current release cadence, we would not have an opportunity to make this breaking change until 2022.

[cwhanse]

I would make this change (switch order of output from get_psm3) in v0.9.

[AdamRJensen]

I've made the change - I really think it's going to be great in the long term having iotools be very consistent!

Now that we're talking iotools inconsistencies and breaking changes - the pvgis_tmy functions do not map variables to pvlib names, which is standard for most other functions including pvgis_hourly. Note the existing renaming dictionary for pvgis_hourly will also work for pvgis_tmy. Given that the output variables would be differently named by default, I'm assuming this is considered a breaking change? Is this something we would also like to do before v0.9? (I'm +1)

[kandersolar]

For PVGIS map_variables can we do a typical deprecation cycle using a strategy like this?

def get_pvgis_tmy(..., map_variables=None):
    if map_variables is None:
        warnings.warn(
            'PVGIS variable names will be renamed to pvlib conventions by '
            'default starting in pvlib 0.10.0. Specify map_variables=True to '
            'enable that behavior now, or specify map_variables=False to '
            'hide this warning.', pvlib._deprecation.pvlibDeprecationWarning
        )
        map_variables = False
    ...

And then in pvlib 0.10.0 we change map_variables to default to True instead of None and remove the warning. Thoughts?

[AdamRJensen]

And then in pvlib 0.10.0 we change map_variables to default to True instead of None and remove the warning. Thoughts?

@kandersolar Is this still the plan? If so, I'll open a PR to change the default and remove warnings.

[kandersolar]

@kandersolar Is this still the plan? If so, I'll open a PR to change the default and remove warnings.

I've not heard anyone say otherwise! Go for it :)