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.

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

Create pvlib.iotools.read_panond for reading .pan and .ond files #1749

Merged
merged 63 commits into from Sep 13, 2023

Conversation

ckrening
Copy link
Contributor

@ckrening ckrening commented May 25, 2023

  • Closes Is there interest in implementing a .pan or .ond reader into iotools? #1747
  • I am familiar with the contributing guidelines
  • Tests added - I tried to add something but was having difficulty finding where to store test files to read.
  • Adds description and name entries in the appropriate "what's new" file in docs/sphinx/source/whatsnew for all changes. Includes link to the GitHub Issue with :issue:`num` or this Pull Request with :pull:`num`. Includes contributor name and/or GitHub username (link with :ghuser:`user`).
  • New code is fully documented. Includes numpydoc compliant docstrings, examples, and comments where necessary.
  • Pull request is nearly complete and ready for detailed review.
  • Maintainer: Appropriate GitHub Labels (including remote-data) and Milestone are assigned to the Pull Request and linked Issue.

First time contributing. So, I may need some extra assistance. I filled out the checklist as best as I could but have left comments where I need some guidance. Please assist with the code documentation, whatsnew comments and setting up appropriate test document.

Proposed change is to add a reader and parser function for .pan and .ond files for the #1747 topic. the reader takes a filepath and creates a nested dictionary from the file contents. I am more than happy to address any concerns in the methods I chose.

@kandersolar
Copy link
Member

Thanks @ckrening! A few initial high-level comments:

@wholmgren
Copy link
Member

I think it might make more sense to name the file pvsyst.py instead of panond.py

Other energy modeling software also accepts pan/ond, so I'm not so sure that we should tie the pvsyst name to the format. PV*SOL documentation contains this interesting tidbit

The file extension PAN has its origin in the French name for solar panel: Panneau solaire. The format was introduced by the simulation software PVSyst and has become internationally accepted as an exchange format for solar modules.

In the long run I'd like to see a set of DataClasses, but I'm good with nested dictionaries in this PR.

@ckrening
Copy link
Contributor Author

Thanks @kandersolar. I made the stickler corrections and updated testing docs.

The only thing on the checklist I am not sure I conform to are the Maintainer: Appropriate GitHub labels.

Finally, I decided to stick with the panond name for the reasons outlined by @wholmgren. I am not attached to panond as a name and it can easily be changed. It's more important to me that the functionality is included.

@mikofski
Copy link
Member

More bike shedding: I’m -1 on panond.py bc it’s just too obscure imo. I’d prefer something more descriptive like components.py

@adriesse
Copy link
Member

What else could go into this module in the future? That could influence the name.

@cwhanse
Copy link
Member

cwhanse commented May 26, 2023

In iotools we've established a pattern of naming the module for the data source, rather than for the type of data being read. For this reason I'm OK with panond. I don't think it's difficult to move the functions to a new module later should we come up with a better name.

About other data sources, I'll note that SAM can write inputs and model configurations to JSON files.

@cwhanse
Copy link
Member

cwhanse commented May 26, 2023

@ckrening is this ready to run tests?

@ckrening
Copy link
Contributor Author

@cwhanse the code is ready to run tests. I have included one sample .pan and .ond.

pvlib/iotools/panond.py Outdated Show resolved Hide resolved
Connor and others added 2 commits June 6, 2023 09:08
@ckrening
Copy link
Contributor Author

@kandersolar regarding the 'End of...' keys in the dictionary, my goal was to modify the file as little as possible. So, I decided to keep those in the dictionary, in the event someone had a use for them. Though I agree, I cannot think of a great use for them.

@kandersolar kandersolar added the remote-data triggers --remote-data pytests label Sep 13, 2023
@kandersolar
Copy link
Member

I did a bit of doc cleanup and test improvements. I think this is ready to go but I think another review from someone else is warranted. @AdamRJensen and/or @cwhanse?

Copy link
Member

@cwhanse cwhanse left a comment

Choose a reason for hiding this comment

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

LGTM thanks @ckrening and @kandersolar

docs/sphinx/source/user_guide/faq.rst Outdated Show resolved Hide resolved
pvlib/iotools/panond.py Outdated Show resolved Hide resolved
Co-authored-by: Cliff Hansen <cwhanse@sandia.gov>
Comment on lines 48 to 49
def parse_panond(fbuf):
"""
Copy link
Member

Choose a reason for hiding this comment

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

I would vote than we only have a read_panond function and eliminate the parse_panond.

We can always create a parse_panond function in the future, but it's annoying to deprecate it.

I believe the reasoning for having the parse function is to avoid duplicate code when you have a get and read function that need similar data handling.

Copy link
Member

Choose a reason for hiding this comment

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

Good point that no get makes parse less relevant. Aside from supporting get, parse is also helpful when file contents might be already in-memory (retrieved from a database or cloud storage, e.g.) instead of a file on disk. Is that helpful for PAN/OND files? The use cases I can think of feel rather contrived. I will make the parse function private.

pvlib/iotools/panond.py Outdated Show resolved Hide resolved
@kandersolar kandersolar changed the title .pan and .ond reader Create pvlib.iotools.read_panond for reading .pan and .ond files Sep 13, 2023
@kandersolar kandersolar merged commit eb7b34a into pvlib:main Sep 13, 2023
24 of 30 checks passed
@kandersolar
Copy link
Member

Thanks @ckrening !

@AdamRJensen
Copy link
Member

Probably should have added a mention of this new function in the FAQ concerning PAN/OND files.

@kandersolar Should I open a new PR or do you want to reopen this PR?

@kandersolar
Copy link
Member

This PR did edit that FAQ entry :) check latest

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement io remote-data triggers --remote-data pytests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Is there interest in implementing a .pan or .ond reader into iotools?
8 participants