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

Fix naming #43

Merged
merged 43 commits into from
Mar 7, 2020
Merged

Fix naming #43

merged 43 commits into from
Mar 7, 2020

Conversation

bt-
Copy link
Member

@bt- bt- commented Jan 15, 2020

Changing names of functions, attributes, and internal object names to remove unclear abbreviations to address issue #36.

@codecov-io
Copy link

codecov-io commented Jan 15, 2020

Codecov Report

Merging #43 into master will decrease coverage by 0.1%.
The diff coverage is 74.13%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #43      +/-   ##
==========================================
- Coverage   70.67%   70.56%   -0.11%     
==========================================
  Files           3        3              
  Lines        1323     1325       +2     
==========================================
  Hits          935      935              
- Misses        388      390       +2
Impacted Files Coverage Δ
captest/capdata.py 77.39% <74.13%> (-0.15%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4df1283...cda7cd8. Read the comment docs.

CHANGELOG.md Outdated
@@ -13,6 +13,26 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
### Changed
- Filter_pvsyst method filters on IL Pmin, IL Pmax, IL Vmin, and IL Vmax and warns if any of the four are missing. Previously failed if any of the four were missing.
- cp_results returns a warning if the regression formulas of the passed CapData objects do not match instead of warning and continuing.
- pvlib 0.6.3 is required; there are issues introduced by the pvlib 0.7.0 release not yet addressed.
Copy link
Contributor

Choose a reason for hiding this comment

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

@bt- is pvlib v0.7 compatibility something you'd like help with?

Copy link
Member Author

Choose a reason for hiding this comment

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

@cwhanse, Yes, that would be great!

I imagine it will be straightforward to fix. If my memory is correct, the test that failed the Travis CI build with pvlib 0.7 was test_pvlib_system when it called

def pvlib_system(sys):

If it would be helpful, I can add a file to the repo to create a conda environment that will run the tests. The existing environment.yml file does not include pytest and possibly a few other packages used in testing.

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like the failure is caused by the name changes in the SAM inverter file. Change this line to

cec_inverter = cec_inverters['ABB__MICRO_0_25_I_OUTD_US_208__208V_']

I think pvlib v0.7 will work for you then.

Copy link
Member Author

Choose a reason for hiding this comment

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

@cwhanse, thanks for the help. It looks like that change did fix that error, but there were other additional errors. I am going to tackle updates for pvlib 0.7.0 on a separate pull request. I created #44 to track that.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry about the trouble. I can't run the tests locally (ran into some build error for sklearn), if you want to set up a branch with the tests run on travis I'm happy to help with issues.

Copy link
Member Author

Choose a reason for hiding this comment

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

No problem! I was working on setting up a branch for this and I realized, if you are willing to work on the updates for pvlib 0.7 compatibility, that it will be easier if you create a branch and pull request. Travis will automatically run all the tests once you make a pull request. I'm not seeing that you have a fork of the project, so that would be the first step.

Please forgive me if you are clear on the step-by-step already. I'm going to write it out here in case it is helpful and so I can reference this and add it to the formal documentation later.

  1. Fork pvcaptest in github
  2. Clone fork to your computer
  3. Create a new branch locally, I was going to name the branch pvlib070
  4. Make revisions and commit them to the new branch
  5. Push the new branch to your fork on github git push -u origin branch_name
  6. Navigate to the new branch on your fork on github and there will be a button you can click to make a pull request
  7. Once you submit the pull request Travis will run tests and the coverage report will be updated

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm familiar with the process but a recipe and reminders are always appreciated!

@bt- bt- merged commit 81b406b into pvcaptest:master Mar 7, 2020
@bt- bt- deleted the fix_naming branch March 8, 2020 15:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants