Conversation
Update spectral albedo representation
Spectral irrad tools
|
@RDaxini @martin-springer I could use some help with knowing what to fix on the CI here. |
pvdeg/spectral.py
Outdated
| import re | ||
| from datetime import datetime as dt | ||
| from datetime import date | ||
| import bifacial_radiance as br |
There was a problem hiding this comment.
@mjprilliman - You are importing bifacial_radiance here but it's not a current requirement for the repo.
@shirubana - You mentioned not to include bifacial_radiance as a requirement. How would you like to handle this?
@mjprilliman - Once quick suggestion is to move the import into the function that's using the package (spectrally_resolved_irradiance) That way, it's not being required when pvdeg is initialized. That will help you get the tests going here on github. But won't help for tests that actually require the package.
So remove this import here as it's loaded via init.py and move it into the function.
wf and bifacialvf function call
| import numpy as np | ||
| import pandas as pd | ||
| from itertools import product | ||
| import bifacialvf |
There was a problem hiding this comment.
@mjprilliman - this import is causing the problem now
|
I have not reviewed in detail yet. It might be worth having a call to discuss the PR, I'm unsure about a few things that are happening in it. One option to consider though, especially in relation to the extra module requirements, is creating a jupyter notebook rather than a new set of functions. Not saying that a notebook tutorial/example is definitely the best way forward; I'm just throwing an idea out there. With a notebook, we wouldn't need to ship the bifacial packages with pvdeg, so we can avoid the additional testing and maintenance. However, we could still set up a custom testing environment for this specific notebook that does install the required modules. Users who want to run the notebook can also be prompted to install any additional requirements themselves, separately from pvdeg. At the same time, we can still ensure the notebook and the rest of pvdeg work with eachother through the testing regime, using nbval or testbook. |
I'm open to moving the new functions to a notebook, depends on what works best for @MDKempe and @shirubana. What about adding the bifacialvf and bifacial_radiance to project.optional-dependencies, with the imports happening only in the functions using them? |
|
@mjprilliman let's go with the alternate solution of putting the calls inside the function, and not making all of this into a notebook |
|
If not a notebook, I lean towards setting these as optional dependencies as it'll make it easier to set up the test workflow using the preconfigured environment with these dependencies listed @mjprilliman, are your changes to degradation.ipynb intentional or have they just arisen from rerunning the notebook? Same for albedo.json, this file seems to have a life of its own whereby it shows up in the diff as having been changed in a PR when, in reality, it hasn't/shouldn't have been (e.g. #185) |
4 participants
Describe your changes
-Add function for calculating spectral front and rear side irradiance at specific wavelengths
-Output separated by wavelength and by front/rear
-Uses updates in bifacial_radiance (NatLabRockies/bifacial_radiance#576) for pulling spectra from SMARTS
-Uses updates in bifacialvf for sky composition method of separating sky into different components (NatLabRockies/bifacialvf#60)
Issue ticket number and link
Fixes # (issue)
Type of change
Please delete options that are not relevant.
Checklist before requesting a review