Skip to content

compability with pvfactors 1.4 #902

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

Closed
wholmgren opened this issue Feb 21, 2020 · 6 comments · Fixed by #934
Closed

compability with pvfactors 1.4 #902

wholmgren opened this issue Feb 21, 2020 · 6 comments · Fixed by #934

Comments

@wholmgren
Copy link
Member

wholmgren commented Feb 21, 2020

The pvfactors_timeseries function does not work with versions of pvfactors >= 1.3.

First reported in the pvlib google group

@anomam do you think it's worth continuing to maintain this wrapper function? It seemed like it made sense in the early days of pvfactors but you've done a lot since then it's not clear to me why people shouldn't just use pvfactors directly.

@wholmgren wholmgren added this to the 0.7.2 milestone Feb 21, 2020
@wholmgren
Copy link
Member Author

well for various reasons I actually can't get test_bifacial.py to pass using 1.2.2, 1.1, or even the CI suite's pvfactors==1.0.1. Maybe something about incompatibility of earlier versions of pvfactors with python 3.8 and/or pandas 1.0? I'm not up to digging hard on it right now.

@wholmgren
Copy link
Member Author

@CameronTStark I'm not interested in fixing this.

@anomam
Copy link
Contributor

anomam commented Feb 25, 2020

@CameronTStark I'm not interested in fixing this.

Hi @wholmgren , I apologize as I haven't been on top of this at all since the first implementation of pvfactors in pvlib. The version 1.4.1 is very stable and a lot better than what's currently used in pvlib (incredibly faster thanks to complete vectorization and almost no reliance on shapely or pandas anymore, plus more modeling features and stronger architecture), and I'd be happy to spend some time updating pvlib if you're still ok with having pvfactors, but I don't know if you are...

@anomam do you think it's worth continuing to maintain this wrapper function? It seemed like it made sense in the early days of pvfactors but you've done a lot since then it's not clear to me why people shouldn't just use pvfactors directly.

I have a couple of questions that could maybe help the discussion:

  • does pvlib desire bifacial modeling capabilities? I just looked at the bifacial.py module and I don't see anything else or any other model implemented apart from what I did, so maybe there's not much interest...?
  • are there any feedback from users about using the pvfactors wrapper?
  • has it become an issue for pvlib to rely on external packages for models? (apart from me doing a bad job maintaining it so far, sorry)

In my opinion, if it was worth having the older version of pvfactors in the first place, then it's probably even more worth having the latest one as it's a lot better. But that's what I'd like to understand/discuss.

Thanks for your time!

@wholmgren
Copy link
Member Author

I should clarify that my statement that I wasn't interested in fixing it extended only to my own contribution (in response to being assigned the issue), not the general utility of fixing it.

does pvlib desire bifacial modeling capabilities? I just looked at the bifacial.py module and I don't see anything else or any other model implemented apart from what I did, so maybe there's not much interest...?

Yes. The lack of additional models is only due to a lack of time.

are there any feedback from users about using the pvfactors wrapper?

This is not the first time that users have asked about compatibility with newer versions of pvfactors.

has it become an issue for pvlib to rely on external packages for models? (apart from me doing a bad job maintaining it so far, sorry)

I don't think we have enough experience yet to determine this.

In my opinion, if it was worth having the older version of pvfactors in the first place, then it's probably even more worth having the latest one as it's a lot better. But that's what I'd like to understand/discuss.

Makes sense to me. I was somehow under the impression that you might now think that pvlib users should use pvfactors as a standalone package. I don't know where I got that from. Maybe just seeing some features go by on my github notifications.

Thanks @anomam!

@anomam
Copy link
Contributor

anomam commented Feb 26, 2020

Ok, thanks @wholmgren . I will try to get started on this during the week or next week; unfortunately I can't allocate work time to this anymore so I'll try to do it one of these nights

@cwhanse
Copy link
Member

cwhanse commented Feb 26, 2020

@anomam I'll second what @wholmgren says, that linking with pvfactors is worth doing. Several other bifacial modeling options are in the works #863 and #717, because there's value in providing a variety of these models in a common framework. Please let me know if there's something I could do to help here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants