Tutorials for single module and multiple row with irradiance distribu… by shirubana · Pull Request #127 · SunPower/PVMismatch

shirubana · 2020-05-20T19:56:01Z

…tions

Tutorials for single module and multiple row with irradiance distributions

…tions Tutorials for single module and multiple row with irradiance distributions

iansloop

Some lines need to be indented

mikofski · 2020-05-29T08:20:14Z

@shirubana & @iansloop, looks like those long lines are just pep8 max line length violations. If you add stickler ci to pvmismatch, then the checks will fail, and you can enforce pep8 more easily.

Another option might be to use black which I have no idea how to use.

For questions on setting stickler, maybe ask @wholmgren or @cwhanse as they might be able to help.

mikofski · 2020-05-29T08:20:40Z

@iansloop sorry to commando this PR, but are you the new maintainer of PVMismatch?

shirubana · 2020-05-29T14:29:03Z

Do you have to indent jupyter journals too? @iansloop you seem to be looking at the raw jupyter code and not the jupyter journal itself maybe?

Also the .py file was generated as a pair to the jupyter journal automatically. We can just delete it, unless @mikofski do you have any idea if it can be done auotomatically?

iansloop · 2020-05-29T15:49:07Z

@iansloop sorry to commando this PR, but are you the new maintainer of PVMismatch?

No, Chetan suggested I practice PRs by reviewing this one

iansloop · 2020-05-29T15:50:52Z

Do you have to indent jupyter journals too? @iansloop you seem to be looking at the raw jupyter code and not the jupyter journal itself maybe?

Also the .py file was generated as a pair to the jupyter journal automatically. We can just delete it, unless @mikofski do you have any idea if it can be done auotomatically?

I was looking at the raw code, I am unsure of best practice for jupyter and so you are most likely fine

kandersolar · 2020-05-29T16:00:22Z

RdTools uses (will use?) nbsphinx to automatically integrate jupyter notebooks with sphinx docs, just FYI if you aren't familiar with that sphinx extension

mikofski · 2020-05-29T17:28:42Z

Hi @iansloop welcome aboard! Great to have to you! I worked at SunPower from 2010-2017 and overlapped with @chetan201 for a year.

Do you have to indent jupyter journals too?

@shirubana Sorry! I didn't realize these were Jupyter notebooks, so my comments above about stickler & black were irrelevant. In general I think pep8 would apply only to the code cells when viewed in the Jupyter notebook (not raw) but maybe exceptions to the max line length and other pep8 rules are forgivable because the format is more flexible, and the output is not going into production? I think the goal of the "tutorial" is to help users, so as long as it tries to accomplish that goal, then it's great.

Also the .py file was generated as a pair to the jupyter journal automatically. We can just delete it,

IMO yes, please delete the .py and .pkl (is that a pickle?) files, IMO they're not necessary, and only add/commit the .ipynb files.

I was looking at the raw code, I am unsure of best practice for jupyter and so you are most likely fine

A couple of suggestions:

don't view jupyter notebooks in their raw output, and never (or hardly ever?) edit the raw JSON output, just pretend it doesn't exist, or that it's computer binary that you shouldn't touch.
instead view jupyter notebooks using the built-in GitHub viewer, or if there's trouble use nbviewer by entering in the path to the .ipynb file, for example:
- EG: 1 - Beginner - Single Module Example
- & 2 - Intermediate - Row Power Calculation under different irradiances
use the three dots for each .ipynb file in the comparison tab, to expand the options and select "View File" to open the the repo tree at the commit with the file, and GitHub will by default automatically show the Jupyter notebook representation of the raw file. Sometimes for large Jupyter notebooks, the rendering is too slow and you have to click "reload?" or use nbviewer
- 1 - Beginner - Single Module Example
- 2 - Intermediate - Row Power Calculation under different irradiances
Install and use nbdime when viewing diffs of jupyter notebooks on your local computer.
I agree with Kevin, that using nbsphinx to add these to the documentation would be optimal

No, Chetan suggested I practice PRs by reviewing this one

do you have any idea of what the future of PVMismatch at SunPower is? What are your plans? Should we depend on it being maintained, or is it more likely that SunPower will let it languish?

chetan201 · 2020-05-29T17:50:33Z

@mikofski @shirubana
We really appreciate this PR and all the ongoing work on PVMM here.

I am trying to expand the pool of reviewers at SunPower to support this project as much as we can while we figure out the future of it.

Thanks for your guidance to Ian. I will help him get started with Jupyter NB environment.

iansloop

Everything looks good, those irradiance plots look really helpful.

Suggestion: This section of code looks like a duplicate of the section above, from "2 - Intermediate - Row Power Calculation under different irradiances.ipnb".

fix repetition of module creation

shirubana · 2020-06-02T21:47:00Z

Good cath @iansloop just removed that.

Also @mikofski ~ on one of the previous comment. The .pkl has the irradiance values of the row that are being used as input in the journal. I can include a csv if it's better?

S.

iansloop

@chetan201 looks ready for you to review

chetan201 · 2020-06-10T20:49:20Z

@shirubana Yes, CSV would be better if it's just text data. Python pickles are finicky across python versions (even minor releases)
Thanks!

replaced pickle with csv and updated journals

shirubana · 2020-06-11T16:51:08Z

@chetan201 updated now

chetan201 · 2020-06-15T22:58:27Z

docs/tutorials/1 - Beginner - Single Module Example.py

+G=np.array([Gpoat]).transpose()
+H = np.ones([1,cellsx]) 
+array_det = np.dot(G,H) 
+sns.heatmap(array_det, square = True)


@shirubana
Seaborn is currently not in the "requirements.txt". We can choose from these options.
1- Either add it as a dependency
2- make the change to Matplotlib's native matshow to show the same sort of heat map.

It may be preferred to keep the package with as few dependencies as possible but I'd leave it to your discretion.

chetan201 · 2020-06-15T23:09:23Z

@shirubana
One last recommendation I'd make is to move the tutorials to top folder and create a new folder called "tutorials" or "notebooks". I think it would be nice for users to land on the main page and see where to get started. I can also add a link in the README.md to them.

These contributions would really help them do the first touch model very quickly! Thanks once again for your PR.

chetan201 · 2020-09-04T16:45:17Z

@shirubana
any chance we can revisit this and close this PR as you get bandwidth? I am happy to make the requested changes myself to reduce burden.

Tutorials for single module and multiple row with irradiance distribu…

be1462d

…tions Tutorials for single module and multiple row with irradiance distributions

iansloop reviewed May 26, 2020

View reviewed changes

iansloop reviewed Jun 2, 2020

View reviewed changes

Journal 2 small cleanup

ee447e8

fix repetition of module creation

iansloop reviewed Jun 2, 2020

View reviewed changes

csv instead of pickle

b32fb1f

replaced pickle with csv and updated journals

chetan201 self-requested a review June 15, 2020 22:47

chetan201 reviewed Jun 15, 2020

View reviewed changes

Conversation

shirubana commented May 20, 2020

Uh oh!

iansloop left a comment

Choose a reason for hiding this comment

Uh oh!

mikofski commented May 29, 2020

Uh oh!

mikofski commented May 29, 2020

Uh oh!

shirubana commented May 29, 2020

Uh oh!

iansloop commented May 29, 2020

Uh oh!

iansloop commented May 29, 2020

Uh oh!

kandersolar commented May 29, 2020

Uh oh!

mikofski commented May 29, 2020

Uh oh!

chetan201 commented May 29, 2020

Uh oh!

iansloop left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shirubana commented Jun 2, 2020

Uh oh!

iansloop left a comment

Choose a reason for hiding this comment

Uh oh!

chetan201 commented Jun 10, 2020

Uh oh!

shirubana commented Jun 11, 2020

Uh oh!

chetan201 Jun 15, 2020

Choose a reason for hiding this comment

Uh oh!

chetan201 Jun 15, 2020

Choose a reason for hiding this comment

Uh oh!

chetan201 commented Jun 15, 2020

Uh oh!

chetan201 commented Sep 4, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

iansloop left a comment •

edited

Loading