Skip to content

fit_pvsyst_sandia uses a mutable dict keyword argument #1046

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
wholmgren opened this issue Sep 4, 2020 · 3 comments · Fixed by #1048
Closed

fit_pvsyst_sandia uses a mutable dict keyword argument #1046

wholmgren opened this issue Sep 4, 2020 · 3 comments · Fixed by #1048
Labels
Milestone

Comments

@wholmgren
Copy link
Member

the ivtools subpackage has code like this:

# utility.py

constants = {'E0': 1000.0, 'T0': 25.0, 'k': 1.38066e-23, 'q': 1.60218e-19}


# sdm.py
from pvlib.ivtools.utility import constants

def fit_pvsyst_sandia(ivcurves, specs, const=constants, maxiter=5, eps1=1.e-3):

The major thing that concerns me is the use of a mutable default. It's generally better practice to use a pattern like const=None in the signature and then if const is None: const = constants

Other things that I noticed while reporting this:

  • We're starting to consistently follow the recommended practice of formatting module level "constants" in upper case. So constants would become CONSTANTS.

  • utility.py is often called utils.py in other packages. We also have a top level tools.py which could be changed too.

thoughts on the above @cwhanse?

@cwhanse
Copy link
Member

cwhanse commented Sep 4, 2020

I'm OK with both suggestions:

constants -> CONSTANTS, and maybe move out of ivtools to someplace more discoverable?

ivtools\utility.py can be ivtools\utils.py. I don't see the contents as relevant outside of ivtools, at present.

@wholmgren wholmgren added this to the 0.8.0 milestone Sep 4, 2020
@wholmgren wholmgren added the api label Sep 4, 2020
@wholmgren
Copy link
Member Author

constants -> CONSTANTS, and maybe move out of ivtools to someplace more discoverable?

Sounds like the other half of #483. Can we agree on a naming scheme quickly enough to get it into 0.8.0 (#825 is relevant to that aspect). If not, I'll propose that we keep this dictionary where it is for now so that we don't have to worry too much about breaking user code if we change it.

k and q can be pulled from scipy.constants once #1035 is merged.

@mikofski
Copy link
Member

mikofski commented Sep 6, 2020

I've merged #1035

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

Successfully merging a pull request may close this issue.

3 participants