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

BUG: pvconst is not a singleton, inconsistent, doesn't update (GH38) #62

Closed mikofski closed 6 years ago

mikofski commented 6 years ago

fixes #38

mikofski commented 6 years ago

this is a WIP, still one more thing to add from #38 to close this

mikofski commented 6 years ago

okay @chetan201 I think this PR is ready to be merged now, @bmeyers you might be interested in this PR since you reported #38 - so this should PR should do 2 things:

  1. more or less makes sure that all pvconst are the same, and
  2. allows the user to change pvconst.npts anytime to change resolution of IV curve calculations, even after creating a system BUT it still doesn't recalculate the system (unless they call setsuns or settemps), so that's another issue.
mikofski commented 6 years ago

okay, I added PVsystem.update() that basically executes:

self.Isys, self.Vsys, self.Psys = self.calcSystem()
(self.Imp, self.Vmp, self.Pmp,
 self.Isc, self.Voc, self.FF, self.eff) = self.calcMPP_IscVocFFeff()

which is WET code that's repeated 3 times. Anyway, after users changes pvconst.npts they could call pvsys.update() and it will recalc the system.

mikofski commented 6 years ago

@chetan201 I can probably resolve these conflicts if you want, they're probably just artifacts of the squash and merge, since this branch is based on gh59.

chetan201 commented 6 years ago

@mikofski Could we have merged the two PRs then to avoid the conflicts? Just curious!

mikofski commented 6 years ago

@chetan201 too L8, I just did it 😜

mikofski commented 6 years ago

sorry, I didn't see you response, oops. The two PR's were merged already, since gh38 was built on top of gh59, if you look at their history - I think the reason there were conflicts was because you did a "squash-and-merge" versus a "pull-and-merge". When you do a "squash-and-merge" it rebases the PR on your master branch into a single commit, so the commits in gh38 where gh59 was already merged, were "squashed", ie: they didn't exist, the history was replaced with a new single commit. Read more about merging pull requests.

chetan201 commented 6 years ago

@mikofski is this PR still valid then if the PRs were merged? I just reviewed it and I think the commits looked good. I have created a tag based off last merge but holding off on this possible merge now before drafting a release.

mikofski commented 6 years ago

you need to also merge this PR as well. GH59 (#61) only covered up to commit 1b674e1 but this PR (#62) continues on for 7 more commits. Look at them on a graph: gh38

mikofski commented 6 years ago

your call regarding tags, it's okay to release two tags in a row. As soon as you push the tag up to GitHub SunPower/PVMismatch, it will automatically trigger the test, build, and deploy to PyPI and GitHub releases.

If you haven't pushed the tag yet, you can also move it to the latest commit using the -f to force it. Google git push --tags <remote> <branch> to read more.

Don't worry about Anaconda for now, unless you want to, it's a PITA, and I haven't automated it. Let me know if you want to hear more about building conda packages, or you could read about it .yourself of course, 😃

chetan201 commented 6 years ago

@mikofski This one seems to be done, including the release! I wonder if I have followed the sequence of cool release names correctly ! ;)

mikofski commented 6 years ago

Yes I like the name! Thanks. I had to manually upload using twine. Pypi sense to be broken, or else my Travis tokens no longer work 😑