cire-thk / bifacialsimu Goto Github PK

Simulation of bifacial PV

License: BSD 3-Clause "New" or "Revised" License

Python 100.00%

bifacialsimu's Issues

[JOSS] egg-info should be removed

The repository currently has a BifacialSimu.egg-info directory. I suggest deleting it (and potentially adding it to the .gitignore) since it is an artifact of the build and not actually part of the repository's source code.

Cross-reference: openjournals/joss-reviews#4443

FileNotFoundError

FileNotFoundError: [Errno 2] No such file or directory: 'C:\Users\lizhen\anaconda3\envs\bifacialsimu\Lib\Outputs\2022 11 30_17_24_results//radiation_qabs_results.csv'

This error occurs when running both modes 3 and 5.

[JOSS] package version in setup.py is out of date

I see that the latest version on PyPI is 1.0.2, but setup.py still lists 1.0.0:

BifacialSimu/setup.py

Line 29 in 158ca37

version='1.0.0',

Something to consider: rather than manually updating a hardcoded version string in setup.py, what many projects do is to determine the version automatically from the git repository information, in particular git tags. setuptools_scm or versioneer are two ways to do this. Using git tags as the basis for versioning is also nice because it makes it easy to go back to the version of the code for older releases. For example here is the code for pvlib version 0.7.1: https://github.com/pvlib/pvlib-python/tree/v0.7.1, which is easy to find because of the git tag.

Cross-reference: openjournals/joss-reviews#4443

[JOSS] ImportError: cannot import name '_missingKeyWarning' from 'bifacial_radiance.main'

When starting GUI.py in Spyder, I have:
ImportError: cannot import name '_missingKeyWarning' from 'bifacial_radiance.main' (C:\Users\Sidi\anaconda3\lib\site-packages\bifacial_radiance\main.py)
Removing it from Lib\site-packages\bifacial_radiance\module.py solves this issue but why? Is _missingKeyWarning really used?

openjournals/joss-reviews#4443 (comment)

[JOSS] Simulations run in the GUI thread ==> freezing GUI

An issue that I mentioned in the Preview stage: the simulations run in the main GUI thread, which make it unresponsive during long time (even if the spyder console output is seen). Simulations should be run in a working thread using the 'threading' library, and a communication between the two threads can be done, e.g. to show the progress of calculations (like a progessbar in tkinter or a simple label). Using threading will make the GUI part remaining responsive and the simulations can be monitored and even stopped when needed. This makes the user experience much more easier.

openjournals/joss-reviews#4443 (comment)

[JOSS] pvlib and pvfactors not included in Anaconda by default

pvlib and pvfactors are not installed by default in Anaconda, at least in the version I downloaded from the official website.
Therefore I had to install these libs manually using the Anaconda Poweshell prompt:
pip install pvlib
pip install pvfactors

Idem for radiance:
pip install bifacial_radiance

Please update the install procedure accordingly

[openjournals/joss-reviews#4443 (comment)](openjournals/joss-reviews#4443 (comment))

Shapely issue on Mac

When running BifacialSimu on Mac I get the following error:

/Users/evagrommes/Documents/GitHub/BifacialSimu/BifacialSimu_src/Vendor/pvfactors/geometry/base.py:349: ShapelyDeprecationWarning: Setting custom attributes on geometry objects is deprecated, and will raise an AttributeError in Shapely 2.0
self.list_surfaces = list_surfaces
/Users/evagrommes/Documents/GitHub/BifacialSimu/BifacialSimu_src/Vendor/pvfactors/geometry/base.py:350: ShapelyDeprecationWarning: Setting custom attributes on geometry objects is deprecated, and will raise an AttributeError in Shapely 2.0
self.shaded = self._get_shading(shaded)
/Users/evagrommes/Documents/GitHub/BifacialSimu/BifacialSimu_src/Vendor/pvfactors/geometry/base.py:351: ShapelyDeprecationWarning: Setting custom attributes on geometry objects is deprecated, and will raise an AttributeError in Shapely 2.0
self.is_collinear = is_collinear(list_surfaces)
/Users/evagrommes/Documents/GitHub/BifacialSimu/BifacialSimu_src/Vendor/pvfactors/geometry/base.py:352: ShapelyDeprecationWarning: Setting custom attributes on geometry objects is deprecated, and will raise an AttributeError in Shapely 2.0
self.param_names = param_names
/Users/evagrommes/Documents/GitHub/BifacialSimu/BifacialSimu_src/Vendor/pvfactors/geometry/base.py:937: ShapelyDeprecationWarning: Setting custom attributes on geometry objects is deprecated, and will raise an AttributeError in Shapely 2.0
self.list_segments = tuple(list_segments)
/Users/evagrommes/Documents/GitHub/BifacialSimu/BifacialSimu_src/Vendor/pvfactors/geometry/base.py:938: ShapelyDeprecationWarning: Setting custom attributes on geometry objects is deprecated, and will raise an AttributeError in Shapely 2.0
self._all_surfaces = None

[JOSS] AUTHORS file

Please add an AUTHORS file to the repo, with the list you gave in Readme.md, with their contribution.
They can also be added in the acknowledgements section.

openjournals/joss-reviews#4443 (comment)

[JOSS] Using modified versions of pvfactors and bifacial_radiance seems unsustainable

The installation instructions in the README say to install pvfactors and bifacial_radiance, then edit those installations to use modified files in this repository. This approach seems problematic to me for a few reasons:

It makes installation more manual and error-prone.
It is brittle: the original projects will continue to evolve and diverge from the old code in this repository. Maybe someday they will diverge enough that your modified code becomes incompatible with the latest version of pvfactors or bifacial_radiance and the code breaks.
By replacing their code with yours, it means any improvements they make to those packages might not apply to your code. For example maybe someday someone finds a bug in pvfactors's pvground.py and fixes it. BifacialSimu won't get the bugfix because it is overwriting that file with an old version from before the bugfix.
It isn't aligned with the open-source ethos of sharing improvements with the wider community. Ideally, any improvements you make to a dependency would be contributed to the dependency itself so that everyone can benefit from it.

If you compare the bifacial_radiance main.py in this repository with the latest version in their repository (here), there are already many differences. I am worried that using modified files will not be sustainable in the long term, and maybe not even in the short term.

Would it be possible to either get rid of those changes, or to contribute those changes back to the original projects? Maybe there is some other solution to this problem?

Cross-reference: openjournals/joss-reviews#4443

[JOSS] Does the package have standard tests?

One of the JOSS review checklist items is about software testing:

Automated tests: Are there automated tests or manual steps described so that the functionality of the software can be verified?

Having some kind of standard suite of tests has a lot of benefits for scientific software packages, including but not limited to:

preventing unintentional changes to simulation results by checking that the software output doesn't change over time
detecting incompatibilities with dependencies (e.g. perhaps tomorrow pvlib releases a new version that breaks your code)
making it easier for new users to ensure they have installed the software correctly on their computer

What many other packages do, including pvlib, pvfactors, and bifacial_radiance, is have a suite of tests covering most or nearly all of the primary package code. We also run these tests automatically for each commit to give us confidence that we are not accidentally breaking something. Using pvlib as an example:

test definitions: https://github.com/pvlib/pvlib-python/tree/master/pvlib/tests
example automated test run: https://github.com/pvlib/pvlib-python/actions/runs/2503197724

In my opinion, testing is essentially a requirement for robust and reliable scientific software. For pvlib we take it so seriously that nearly 100% of the package code is covered by tests (see https://app.codecov.io/gh/pvlib/pvlib-python) and we don't accept new code unless it has tests (see pvlib's contributing docs).

Does BifacialSimu have anything along these lines? I didn't see any tests, but maybe I missed it. If not, do you have any plans to implement tests?

Cross-reference: openjournals/joss-reviews#4443

[JOSS] ViewFactors error: "dateutil.parser._parser.ParserError: Unknown string format: 11-01 00:00%"

To reproduce:

Start GUI
Hit "set default!"
In "Simulation Control", select "front and back simulation with Viewfactors"
Hit "start simulation"

Full traceback:

Click to expand!

Exception in thread Thread-11:
Traceback (most recent call last):
  File "/home/kevin/miniconda3/envs/bifacialsimu-review/lib/python3.8/site-packages/pandas/core/arrays/datetimes.py", line 2187, in objects_to_datetime64ns
    values, tz_parsed = conversion.datetime_to_datetime64(data.ravel("K"))
  File "pandas/_libs/tslibs/conversion.pyx", line 359, in pandas._libs.tslibs.conversion.datetime_to_datetime64
TypeError: Unrecognized value type: <class 'str'>

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/home/kevin/miniconda3/envs/bifacialsimu-review/lib/python3.8/threading.py", line 932, in _bootstrap_inner
    self.run()
  File "/home/kevin/miniconda3/envs/bifacialsimu-review/lib/python3.8/threading.py", line 870, in run
    self._target(*self._args, **self._kwargs)
  File "/home/kevin/projects/BifacialSimu/BifacialSimu_src/GUI.py", line 562, in StartSimulation
    Controller.startSimulation(SimulationDict, ModuleDict, resultsPath)
  File "/home/kevin/projects/BifacialSimu/BifacialSimu_src/BifacialSimu/Controller/BifacialSimu_simulationController.py", line 91, in startSimulation
    df_reportVF, df = BifacialSimu_radiationHandler.ViewFactors.simulateViewFactors(simulationDict, demo, metdata,  df, resultsPath, onlyFrontscan = False)
  File "/home/kevin/projects/BifacialSimu/BifacialSimu_src/BifacialSimu/Handler/BifacialSimu_radiationHandler.py", line 580, in simulateViewFactors
    df['timestamp'] = pd.to_datetime(df['timestamp'])  
  File "/home/kevin/miniconda3/envs/bifacialsimu-review/lib/python3.8/site-packages/pandas/core/tools/datetimes.py", line 887, in to_datetime
    values = convert_listlike(arg._values, format)
  File "/home/kevin/miniconda3/envs/bifacialsimu-review/lib/python3.8/site-packages/pandas/core/tools/datetimes.py", line 401, in _convert_listlike_datetimes
    result, tz_parsed = objects_to_datetime64ns(
  File "/home/kevin/miniconda3/envs/bifacialsimu-review/lib/python3.8/site-packages/pandas/core/arrays/datetimes.py", line 2193, in objects_to_datetime64ns
    raise err
  File "/home/kevin/miniconda3/envs/bifacialsimu-review/lib/python3.8/site-packages/pandas/core/arrays/datetimes.py", line 2175, in objects_to_datetime64ns
    result, tz_parsed = tslib.array_to_datetime(
  File "pandas/_libs/tslib.pyx", line 379, in pandas._libs.tslib.array_to_datetime
  File "pandas/_libs/tslib.pyx", line 611, in pandas._libs.tslib.array_to_datetime
  File "pandas/_libs/tslib.pyx", line 749, in pandas._libs.tslib._array_to_datetime_object
  File "pandas/_libs/tslib.pyx", line 740, in pandas._libs.tslib._array_to_datetime_object
  File "pandas/_libs/tslibs/parsing.pyx", line 257, in pandas._libs.tslibs.parsing.parse_datetime_string
  File "/home/kevin/miniconda3/envs/bifacialsimu-review/lib/python3.8/site-packages/dateutil/parser/_parser.py", line 1368, in parse
    return DEFAULTPARSER.parse(timestr, **kwargs)
  File "/home/kevin/miniconda3/envs/bifacialsimu-review/lib/python3.8/site-packages/dateutil/parser/_parser.py", line 643, in parse
    raise ParserError("Unknown string format: %s", timestr)
dateutil.parser._parser.ParserError: Unknown string format: 11-01 00:00%

I think this line (and a similar one several lines farther down) has an extra % character:

BifacialSimu/BifacialSimu_src/BifacialSimu/Handler/BifacialSimu_radiationHandler.py

Line 562 in 158ca37

df['timestamp'] = df['corrected_timestamp'].dt.strftime('%m-%d %H:%M%')

But even removing that extra %, the datetime is still not parsed correctly:

Click to expand!

Exception in thread Thread-11:
Traceback (most recent call last):
  File "/home/kevin/miniconda3/envs/bifacialsimu-review/lib/python3.8/site-packages/pandas/core/arrays/datetimes.py", line 2187, in objects_to_datetime64ns
    values, tz_parsed = conversion.datetime_to_datetime64(data.ravel("K"))
  File "pandas/_libs/tslibs/conversion.pyx", line 359, in pandas._libs.tslibs.conversion.datetime_to_datetime64
TypeError: Unrecognized value type: <class 'str'>

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/home/kevin/miniconda3/envs/bifacialsimu-review/lib/python3.8/threading.py", line 932, in _bootstrap_inner
    self.run()
  File "/home/kevin/miniconda3/envs/bifacialsimu-review/lib/python3.8/threading.py", line 870, in run
    self._target(*self._args, **self._kwargs)
  File "/home/kevin/projects/BifacialSimu/BifacialSimu_src/GUI.py", line 562, in StartSimulation
    Controller.startSimulation(SimulationDict, ModuleDict, resultsPath)
  File "/home/kevin/projects/BifacialSimu/BifacialSimu_src/BifacialSimu/Controller/BifacialSimu_simulationController.py", line 91, in startSimulation
    df_reportVF, df = BifacialSimu_radiationHandler.ViewFactors.simulateViewFactors(simulationDict, demo, metdata,  df, resultsPath, onlyFrontscan = False)
  File "/home/kevin/projects/BifacialSimu/BifacialSimu_src/BifacialSimu/Handler/BifacialSimu_radiationHandler.py", line 580, in simulateViewFactors
    df['timestamp'] = pd.to_datetime(df['timestamp'])  
  File "/home/kevin/miniconda3/envs/bifacialsimu-review/lib/python3.8/site-packages/pandas/core/tools/datetimes.py", line 887, in to_datetime
    values = convert_listlike(arg._values, format)
  File "/home/kevin/miniconda3/envs/bifacialsimu-review/lib/python3.8/site-packages/pandas/core/tools/datetimes.py", line 401, in _convert_listlike_datetimes
    result, tz_parsed = objects_to_datetime64ns(
  File "/home/kevin/miniconda3/envs/bifacialsimu-review/lib/python3.8/site-packages/pandas/core/arrays/datetimes.py", line 2193, in objects_to_datetime64ns
    raise err
  File "/home/kevin/miniconda3/envs/bifacialsimu-review/lib/python3.8/site-packages/pandas/core/arrays/datetimes.py", line 2175, in objects_to_datetime64ns
    result, tz_parsed = tslib.array_to_datetime(
  File "pandas/_libs/tslib.pyx", line 379, in pandas._libs.tslib.array_to_datetime
  File "pandas/_libs/tslib.pyx", line 606, in pandas._libs.tslib.array_to_datetime
  File "pandas/_libs/tslib.pyx", line 602, in pandas._libs.tslib.array_to_datetime
  File "pandas/_libs/tslib.pyx", line 557, in pandas._libs.tslib.array_to_datetime
  File "pandas/_libs/tslibs/conversion.pyx", line 516, in pandas._libs.tslibs.conversion.convert_datetime_to_tsobject
  File "pandas/_libs/tslibs/np_datetime.pyx", line 120, in pandas._libs.tslibs.np_datetime.check_dts_bounds
pandas._libs.tslibs.np_datetime.OutOfBoundsDatetime: Out of bounds nanosecond timestamp: 1-11-01 00:00:00

It seems to me like the year should be included with %Y, otherwise how can pd.to_datetime know what year it is?

Cross-reference: openjournals/joss-reviews#4443

[JOSS] paper.md feedback

Here are some notes I took while reading the paper.md manuscript file. Hopefully this feedback is helpful!

Statement of need

PV is the third largest power generation technology and will play a significant role in future global electricity generation (International Renewable Energy Agengy, 2019).

I read this as claiming that solar is currently the third largest power generation technology, which I don't think is what the sentence was intended to convey. Maybe something like "Current forecasts expect PV to be the third largest power generation technology by 2050..." would be better?

There is a need for an open source software tool to simulate bifacial PV, which combines existing radiation and electrical models and provides a graphical user interface (GUI).

I think this deserves more explanation. Why is the combination of viewfactor and raytracing desirable? Why not just use one or the other? I would also argue that albedo should be discussed here. Why are advanced albedo models necessary? I'm not saying they aren't necessary, but that it would strengthen the paper to discuss it.

This comment is a little more vague, but I as the reader am not sure what the main "selling point" of BifacialSimu is, or why I might want to use it instead of some other tool. Here are the questions I'm asking myself as prospective user:

Is it for more accurate system performance modeling? No, since it doesn't include all the auxiliary things needed to model system performance (soiling, inverters, etc).
Is it to make bifacial irradiance modeling easier for non-programmers? No, since other bifacial tools (bifacial_radiance, SAM) also have GUIs.
Is it because it's open-source? No, because the underlying irradiance models are also open-source.

Tied in with this is the question "why create a new tool instead of improving an existing tool?". For example someone could say that it would have been better to add fancy albedo models to bifacial_radiance, and improve its existing gui, instead of making a new wrapper around it. If you can, it would be good to clarify what exactly the need is and why these other existing tools don't address it.

State of the field

BifacialSimu offers the advantage of an integrated energy yield calculation, whereas bifacial_radiance need a third software, the open-source software PVMismatch [@Mikofski.2018].

I think this is no longer true; my understanding is that bifacial_radiance now includes its own PV I-V calculations using pvlib. See for example their Tutorial 21.

The commonly known open-source software using the viewfactor model, pvfactors [@Anoma.2017] and bifacialVF [@Marion.2017], have both no energy yield calculation and no GUI, which limits the target audience and the application possibilities. The commonly used software PVsyst offers an energy yield calculation and a GUI, but the used physical models are only documented to a limited extent and the software must be paid [@PVsystSA].

NREL's System Advisor Model has a viewfactor-based rear-side irradiance model, does energy yield calculation, has a GUI, is well-documented, and is open source, so if those are the relevant criteria then it's probably worth mentioning SAM here.

I also think BifacialSimu's advanced albedo modeling should be highlighted in this State of the field section, since that's a clear technical advantage compared to other packages.

Cross-reference: openjournals/joss-reviews#4443

[JOSS] No community guidelines

One of the JOSS review checklist items is about instructions for users and contributors:

Community guidelines: Are there clear guidelines for third parties wishing to 1) Contribute to the software 2) Report issues or problems with the software 3) Seek support

I think these points are not really addressed in the README or anywhere else. Here are some examples from other packages if you'd like some inspiration:

Cross-reference: openjournals/joss-reviews#4443

[JOSS] document all dependencies

One of the JOSS review checklist items is about dependencies:

Is there a clearly-stated list of dependencies? Ideally these should be handled with an automated package management solution.

I think right now the answer to that question is "no". Looking through the package files, I see that, in addition to the dependencies explicitly mentioned in the README, BifacialSimu requires these external packages:

pandas
IPython
matplotlib
numpy
tqdm
webbrowser
dateutil
seaborn (optional)

I think at least some of these are not provided by the default Anaconda distribution, so they should be at least mentioned in the README installation instructions.

However, I agree with the checklist item that it would be better to use an automated package manager to track dependencies. In the python world you can use setup.py or setup.cfg for this, which would make your package compatible with tools like pip. You could also list it on pypi.org if you wanted.

Cross-reference: openjournals/joss-reviews#4443

[JOSS] GUI issues on linux

I ran into two issues with BifacialSimu_src/GUI.py on linux. The first is that the filepaths for logos, help text, configuration files, etc use hardcoded backslashes, but linux (and Mac OS) use forward slashes, and so I get errors like this:

TclError: couldn't open "/home/kevin/projects/BifacialSimu/BifacialSimu_src\Lib\Button_Images\Button-Info-icon.png": no such file or directory

and

FileNotFoundError: [Errno 2] No such file or directory: '/home/kevin/projects/BifacialSimu/BifacialSimu_src\\Lib\\input_albedo\\Albedo.json'

Editing GUI.py to use forward slashes gets it working for me, but of course that might break on Windows. To be cross platform you can use os.path.join or the pathlib standard library (new in python 3.4).

The second issue is that tkinter doesn't like the .ico files even after fixing the filepath. For example I get this:

_tkinter.TclError: bitmap "/home/kevin/projects/BifacialSimu/BifacialSimu_src/Lib/Button_Images/info_logo.ico" not defined

But os.path.exists confirms the file is there, so it must be some incompatibility that I don't understand. Anyway this prevents some things from working, for example the button_SP() window is empty until I comment out the pop_SP.iconbitmap(...) line. I did not try to find a better fix since I assume omitting the icons will not break any functionality.

Cross-reference: openjournals/joss-reviews#4443

[JOSS] git repository contains unnecessary files

There are a handful of __pycache__ folders and .DS_Store files scattered around the repository. Since these are unnecessary and essentially just clutter, I suggest removing them and creating a .gitignore file to prevent them from coming back.

Cross-reference: openjournals/joss-reviews#4443

cire-thk / bifacialsimu Goto Github PK

bifacialsimu's Issues

Statement of need

State of the field

Recommend Projects

Recommend Topics

Recommend Org