search

SunPower / PVMismatch

An explicit Python PV system IV & PV curve trace calculator which can also calculate mismatch.
http://sunpower.github.io/PVMismatch/
BSD 3-Clause "New" or "Revised" License
79 stars 30 forks source link

Contributing the Module Mismatch Simulation script #47

Closed bmeyers closed 7 years ago

bmeyers commented 7 years ago

@mikofski ready for review

bmeyers commented 7 years ago

@mikofski you still interested in adding this to v3 as well?

mikofski commented 7 years ago

Yep, but I got other fish to fry. Is there a rush on it? Do you need this?

bmeyers commented 7 years ago

nope, no rush. just wanted to get it out there. did you want to review the code first?

bmeyers commented 7 years ago

@mikofski can i merge this, or should there be a review first?

mikofski commented 7 years ago

IMO, there should always be at least one review. What's your opinion?

bmeyers commented 7 years ago

Fine by me, I just wasn't sure what you had in mind in terms of a review for a stand-alone contribution like this.

bmeyers commented 7 years ago

on it

bmeyers commented 7 years ago

Looks clean to me know. Only difference from master is the new script.

mikofski commented 7 years ago

Yep, it's much easier to read now. Thanks, I'm reviewing it now.

mikofski commented 7 years ago

Unfortunately it doesn't seem to work with the latest matplotlib and numpy? matplotlib (2.0.0) numpy (1.12.0+mkl)

Here's the traceback

Here's a screenshot: pvmismatch_module_simulator

bmeyers commented 7 years ago

Interesting, I will double check my version of both and do some testing.

And i hear you loud and clear on the issue of altering path. I'll simply have the exception inform the user that pvmismatch needs to be on their path before they can run the script and suggest the solutions you describe.

mikofski commented 7 years ago

IMO, you shouldn't need that try: ... except: ... at all because if the import fails, then Python will already raise an ImportError. We want an exception here, and it's already being handled, and it actually already tells the user everything they need to know to fix the problem

bmeyers commented 7 years ago

I just liked the idea of giving a little feedback to the user. In my early days, I had some confusion on that point, and I thought what your wrote was helpful.

bmeyers commented 7 years ago

@mikofski I believe I have addressed all the comments. Thank you for giving this such a thorough review! I appreciate the feedback. Learning a lot over here.

mikofski commented 7 years ago

OK @bmeyers can you tag this as v3.0 using git and push the tags to upstream

$ git push --tags upstream

then can you build a new wheel and a new tarball and upload them to pypi.

(venv) $ python setup.py bdist_wheel sdist upload

there is no need to update the docs at all, so leave them alone.

you might want to upload the wheel and tarball to github releases as well, they're in dist/

then let's work on issue #49