Skip to content

Change method of multi-inheritance to allow kwargs in parent class of LocalizedPVSystem and LocalizedSingleAxisTracker #330

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

Merged
merged 9 commits into from
Jun 12, 2017

Conversation

uvchik
Copy link
Contributor

@uvchik uvchik commented Jun 2, 2017

If keyword arguments are present a TypeError will be raised, so it is better to remove it from the super call.

As I normally initialise classes using a parameter dictionary, so it often happens that I will have additional keys. Therefore it is convenient if the PVSystem class takes **kwargs but they should not be passed on.

@uvchik
Copy link
Contributor Author

uvchik commented Jun 2, 2017

My fault, so I reverted the commit.

For me it would be nice to make it possible to add additional parameters to the dictionary and pass it using the double star operator (see below). But may be it was a deliberate decision to forbid superfluous parameter.

location = {
        'altitude': 34,
        'latitude': 50,
        'longitude': 15}

system = {
    'module_parameters': sandia_modules[module_name],
    'inverter_parameters': sapm_inverters[inverter_name],
    'surface_azimuth': 180,
    'surface_tilt': 30,
    'albedo': 0.2,
    'additional_information': 'something' 
    }
mc = ModelChain(PVSystem(**system), Location(**location))

If you want to allow additional parameters in this class I could think about, how to do it. If not just close this PR. I do not use it all the time, but I like it especially for system parameters.

@wholmgren
Copy link
Member

I've run into this issue, as well, but haven't given much thought to the best approach. I am open to changing things, though.

I have overseen this class so I had to change it in the same way as in
commit 6d1beaf.
@uvchik
Copy link
Contributor Author

uvchik commented Jun 6, 2017

What I read about multiple inheritance, one can call the init-methods of the parent classes explicitly to avoid such behaviour. After I have changed everything, all tests work fine, except of one. But I think the test is wrong and not my changes, because my name is passed to the method and so the expected name-attribute should be my name and not None.

@wholmgren May you check, if I changed the test correctly (commit: 37d84ec).

@wholmgren
Copy link
Member

Thanks. After (re)reading a bunch of stuff on multiple inheritance in python, I think your proposal is a better way to handle these classes.

I agree that the test asserts the wrong result. It looks like the PVSystem.__init__ consumes the name kwarg before it gets to Location, then the Location__init__ sets the name to None. Your code passes all kwargs to both __init__ methods, so the composite object ends up with the right attribute value regardless of the initialization order.

Can you start a 0.4.6 whats new file and describe this fix?

@wholmgren wholmgren added this to the 0.4.6 milestone Jun 10, 2017
@uvchik
Copy link
Contributor Author

uvchik commented Jun 12, 2017

Thank you for your feedback.

I added a whatsnew file as .rst as it is a reStructuredText file. This makes file handling a little easier.

I hope I described my changes comprehensible.

~~~~~~~~~

* Method of multi-inheritance has changed to make it possible to use kwargs in
the parent class (:issue:`330`)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should probably specify that this is for LocalizedPVSystem and LocalizedSingleAxisTracker.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would also be good to update the PR name to something like "allow kwargs in parent class of LocalizedPVSystem and LocalizedSingleAxisTracker"

@uvchik uvchik changed the title remove superfluous keyword arguments Change method of multi-inheritance to allow kwargs in parent class of LocalizedPVSystem and LocalizedSingleAxisTracker Jun 12, 2017
@uvchik
Copy link
Contributor Author

uvchik commented Jun 12, 2017

Is it okay like this?

@wholmgren
Copy link
Member

Yes, thanks!

@wholmgren wholmgren merged commit ac4ca6f into pvlib:master Jun 12, 2017
@wholmgren wholmgren modified the milestones: 0.4.6, 0.5.0 Aug 8, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants