Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Tutorials for single module and multiple row with irradiance distribu… #127

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Tutorials for single module and multiple row with irradiance distribu… #127

wants to merge 3 commits into from

Conversation

shirubana
Copy link

…tions

Tutorials for single module and multiple row with irradiance distributions

@shirubana
Tutorials for single module and multiple row with irradiance distribu…
be1462d 
…tions

Tutorials for single module and multiple row with irradiance distributions
iansloop
iansloop reviewed May 26, 2020
View reviewed changes
Copy link

@iansloop iansloop left a comment

Choose a reason for hiding this comment

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

Some lines need to be indented

@mikofski
Copy link
Contributor

@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
Copy link
Contributor

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

@shirubana
Copy link
Author

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
Copy link

@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
Copy link

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
Copy link
Contributor

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
Copy link
Contributor

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:

  • 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

  • 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
Copy link
Contributor

@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
iansloop reviewed Jun 2, 2020
View reviewed changes
Copy link

@iansloop iansloop left a comment •
edited
Loading

Choose a reason for hiding this comment

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

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".

image

@shirubana
Journal 2 small cleanup
ee447e8 
fix repetition of module creation
@shirubana
Copy link
Author

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
iansloop reviewed Jun 2, 2020
View reviewed changes
Copy link

@iansloop iansloop left a comment

Choose a reason for hiding this comment

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

@chetan201 looks ready for you to review

@chetan201
Copy link
Contributor

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

@shirubana
csv instead of pickle
b32fb1f 
replaced pickle with csv and updated journals
@shirubana
Copy link
Author

@chetan201 updated now

@chetan201 chetan201 self-requested a review June 15, 2020 22:47
chetan201
chetan201 reviewed Jun 15, 2020
View reviewed changes
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)
Copy link
Contributor

Choose a reason for hiding this comment

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

image

Copy link
Contributor

Choose a reason for hiding this comment

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

@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
Copy link
Contributor

@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
Copy link
Contributor

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@iansloop iansloop iansloop left review comments

@chetan201 chetan201 chetan201 left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@shirubana @mikofski @iansloop @kandersolar @chetan201