Skip to content

"add __repr__() function to class PVSystem, LocalizedPVSystem, ModelChain, SingleAxisTracker, Location and add test for same" #174

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

Merged
merged 10 commits into from
Jun 10, 2016

Conversation

JohannesOos
Copy link
Contributor

According to issue #142

@wholmgren
Copy link
Member

Thanks for submitting this. I'd lean towards including more attributes for PVSystem and add some for LocalizedPVSystem, but I don't have a specific recommendation for which to include and exclude.

Also, please keep the line length to 79 or fewer characters.

@wholmgren wholmgren added this to the 0.3.3 milestone May 16, 2016
@JohannesOos
Copy link
Contributor Author

Thanks for advise.
Updated length of lines.
Updated more output and added self.location for LocalizedPVSystem

@wholmgren
Copy link
Member

Please consolidate this PR and #175. The PR will automatically be updated when you push new commits to GitHub, so there's no need to create a second one for the same topic.

We'll need tests similar to this one:

https://github.com/pvlib/pvlib-python/blob/master/pvlib/test/test_location.py#L37

@JohannesOos
Copy link
Contributor Author

Thanks very much for the advise. Am new to the github workflow, so thanks for the patience.
Closed #175 and put changed on this thread.

@JohannesOos
Copy link
Contributor Author

Regarding testing:
No experience with it, so will leave it open.

@@ -8,7 +8,7 @@

import pandas as pd

from pvlib import solarposition, pvsystem, clearsky, atmosphere
from pvlib import solarposition, pvsystem, clearsky, atmosphere, location
Copy link
Member

Choose a reason for hiding this comment

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

Is this new import needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right, there is no need for that import

@wholmgren
Copy link
Member

Running the tests should be as easy as: conda or pip install nose, then run nosetests pvlib -v from your pvlib-python directory. I think that you can create tests for your new methods fairly easily by following the simple test example linked to above. I encourage you to try it.

@JohannesOos
Copy link
Contributor Author

Thanks for the guidance on the testing - I did pattern matching from the example. The test worked on my machine, not sure though.
added the tests for ModelChain.repr(), Location.repr(), PVSystem.repr() and LocalizedPVSystem.repr().

@JohannesOos JohannesOos changed the title "add __repr__ function to class PVSystem and LocalizedPVSystem" "add __repr__() function to class PVSystem, LocalizedPVSystem, ModelChain, Location and add test for same" May 19, 2016
@wholmgren
Copy link
Member

The tests look good, though we still need a couple for the trackers.

Did you see the comments about the unnecessary location import and undesirable location attribute? They should show up in-line here, otherwise you can look at the files changed tab.

@JohannesOos
Copy link
Contributor Author

Added the tests of repr() for the trackers

@JohannesOos
Copy link
Contributor Author

Location in trackers adjusted - did not realize the override before. Thanks!

@JohannesOos JohannesOos changed the title "add __repr__() function to class PVSystem, LocalizedPVSystem, ModelChain, Location and add test for same" "add __repr__() function to class PVSystem, LocalizedPVSystem, ModelChain, SingleAxisTracker, Location and add test for same" May 21, 2016
@wholmgren
Copy link
Member

You'll need to make the same changes to your LocalizedPVSystem: remove self.location = location and add the individual attributes to the repr. You also added a location import to modelchain that isn't used and should be removed.

Is there any reason to keep the Location.__str__ method?

Finally, you're going to need to rebase your commits on the latest version pvlib and resolve a merge conflict along the way. See this comment and google for more.

@JohannesOos
Copy link
Contributor Author

Did the rebase, but think I killed some of my changed on the way. So will re-commit the modelchain and location file

@wholmgren
Copy link
Member

@JohannesOos we're close... I'm still hoping to get rid of the self.location assignment in LocalizedPVSystem. Also, did you see that one of your tests is failing?

@JohannesOos
Copy link
Contributor Author

Adjusted the tests and replaced self.location with the lat and lon attributes inherited from location.
Thanks for the continuous advise.

@wholmgren
Copy link
Member

Looks good to me. I'll merge tomorrow unless anyone objects.

@JohannesOos
Copy link
Contributor Author

Cool, thanks for the help on getting me up to speed with github

@wholmgren wholmgren merged commit 5b56dae into pvlib:master Jun 10, 2016
@wholmgren
Copy link
Member

Thanks @JohannesOos!

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

Successfully merging this pull request may close these issues.

2 participants