add consistency tests to ModelChain.dc_model

[cwhanse]

pvlib python pull request guidelines

Thank you for your contribution to pvlib python! You may delete all of these instructions except for the list below.

You may submit a pull request with your code at any stage of completion.

The following items must be addressed before the code can be merged. Please don't hesitate to ask for help if you're unsure of how to accomplish any of the items below:

• Closes issue Catch key errors when selecting from module databases #417
• I am familiar with the contributing guidelines.
• Fully tested. Added and/or modified tests to ensure correct behavior for all reasonable inputs. Tests (usually) must pass on the TravisCI and Appveyor testing services.
• Updates entries to docs/sphinx/source/api.rst for API changes.
• Adds description and name entries in the appropriate docs/sphinx/source/whatsnew file for all changes.
• Code quality and style is sufficient. Passes git diff upstream/master -u -- "*.py" | flake8 --diff
• New code is fully documented. Includes sphinx/numpydoc compliant docstrings and comments in the code where necessary.
• Pull request is nearly complete and ready for detailed review.

Brief description of the problem and proposed solution (if not already fully described in the issue linked to above):

Tests for consistency between module parameters and dc_model choice.

[wholmgren]

self.system.module_parameters.keys()

[wholmgren]

do you want to use this whole set in the ModelChain.infer_dc_model below?

[cwhanse]

yes

[wholmgren]

I think you can get by without the + \. Strings are automatically concatenated within () that span multiple lines.

[wholmgren]

I'm ok with using a function for each of these but it seems a bit overkill and harder to extend in the future.

One alternative would be defining a class or module level dictionary like this:

dc_model_parameters = {'sapm': set([....]), 'singlediode': set([...]), 'pvwatts': set([...])}

[wholmgren]

One could argue that this parameter dictionary should live in pvsystem.py and imported into modelchain.py

[cwhanse]

At what point in modelchain.py should this function be imported?

[wholmgren]

Probably at the top with the rest of the imports.

[cwhanse]

I'm considering the following change, in which infer_dc_model is always followed by the logic in dc_model to validate the parameters. To me this is better than having infer_dc_model do its own consistency check on the full parameter list.

    def dc_model(self, model):

       # guess at model if None
        if model is None:
            self._dc_model, model = self.infer_dc_model()
        # Set model and validate parameters
        if isinstance(model, str):
            model = model.lower()
            # validate module parameters
            missing_params = DC_MODEL_PARAMS[model] - \
                                    set(self.system.module_parameters.keys())
            if missing_params: # some parameters are not in module.keys()
                raise ValueError(model + ' selected for the DC model but '
                                     'one or more required parameters '
                                     ' are missing : ' +
                                     str(missing_params))
            if model == 'sapm':
                self._dc_model = self.sapm
            elif model == 'singlediode':
                self._dc_model = self.singlediode
            elif model == 'pvwatts':
                self._dc_model = self.pvwatts_dc
            else:
                raise ValueError(model + ' is not a valid DC power model')
        else:
            self._dc_model = partial(model, self)

    def infer_dc_model(self):
        # returns both model function object and model string, could drop
        # model function object in the future since the model function object
        # will be set in dc_model after validating parameter consistency
        params = set(self.system.module_parameters.keys())
        if set(['A0', 'A1', 'C7']) <= params:
            return self.sapm, 'sapm'
        elif set(['a_ref', 'I_L_ref', 'I_o_ref', 'R_sh_ref', 'R_s']) <= params:
            return self.singlediode, 'singlediode'
        elif set(['pdc0', 'gamma_pdc']) <= params:
            return self.pvwatts_dc, 'pvwatts_dc'
        else:
            raise ValueError('could not infer DC model from '
                             'system.module_parameters')

[wholmgren]

@cwhanse that's ok with me. Your comment says "could drop model function object in the future" -- why not now?

[cwhanse]

It’s an API change for the method, I’m hesitant to possibly break someone’s code without a warning although the risk here is probably zero.

[wholmgren]

It's an API change either way, but I agree the risk is near 0 and not worth a deprecation cycle.

[adriesse]

Just picking up on the last few comments out of context...

I think it would be nice to users to generate warnings related to API changes, provided you can detect that they're trying to use something form the old API, and to keep those warnings around for a good long time. This will be especially useful for occasional users, who may not update everything all the time, and new users, who may pick up examples based on any past version, and old users who can't remember stuff.

[wholmgren]

@adriesse 0.6.0 will print deprecation warnings for the vast majority of API changes. I'd like to keep the warnings around "long enough" but I have no idea what that or "a good long time" means in practice! Realistically, we're looking at 6-12 months for the current pace of 0.x releases.

For the specific API change discussed here, I'd rather not go through a deprecation cycle because I don't think anyone is explicitly using this method so It's not worth the effort.

[cwhanse]

Ready to merge, I think. #463 and #487 need to build on top of these changes.

[wholmgren]

space in string at end of previous line and beginning of this one, so remove one of them.

[cwhanse]

Removed

[wholmgren]

See if you can set your editor to remove extra white space from lines when saving. It makes git diffs unnecessarily busy.

[cwhanse]

Done

[wholmgren]

Did you find that it was necessary to use copy here for the test to pass with pytest? I am surprised because the test fixtures should generate a new system with fresh data for each test. You're not reusing them within this test so I don't see why it would be necessary. I can see that if you were developing this test function in the interpreter/notebook then the story would be different.

[cwhanse]

I didn't know that test fixtures are generated anew for each test. I used copy out of concern that pop-ing a key off the module_parameters dict would alter system etc. for other tests.

Removed.