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

pvlib.ivtools.sdm.fit_cec_sam() causes import error due to uppercase letters in import statement #2083

Closed
jngarber-st opened this issue Jun 7, 2024 · 8 comments

Comments

@jngarber-st
Copy link

jngarber-st commented Jun 7, 2024

Describe the bug
Using pvlib.ivtools.sdm.fit_cec_sam() causes an import error, stating there is no package installed named "PySAM". This is using a virtual environment with both pvlib and nrel-pysam installed. The problem happens because python is looking for a package named "PySAM", though the installed package is named "pysam". Changing the import statement to lower case on line 102 of sdm.py fixes the issue. I thought the python standard was for packages to be all lowercase. I'm on MacOS using python 3.12.

To Reproduce
To reproduce, try to use the pvlib.ivtools.sdm.fit_cec_sam() function.

Expected behavior
I expect it to return the estimated CEC SDM parameters. It does this after fixing the import statement.

Versions:

  • pvlib.__version__: 0.10.5
  • pandas.__version__: 2.2.2
  • python: 3.12

Additional context
I can provide a pull request if needed, but felt creating an issue ticket first was more appropriate.

@jngarber-st jngarber-st changed the title pvlib.ivtools.sdm.fit_cec_sam() causes import error due to caps in import statement pvlib.ivtools.sdm.fit_cec_sam() causes import error due to uppercase letters in import statement Jun 7, 2024
@kandersolar
Copy link
Member

I am not able to reproduce this issue on windows; fit_cec_sam() works fine for me, and in recent CI test runs. The CI includes MacOS, but I think the PySAM function is not tested there.

FWIW, their docs say to import it as PySAM: https://nrel-pysam.readthedocs.io/en/main/getting-started.html

How did you install PySAM, and what version do you have installed? Can you provide us a sequence of terminal commands that produce a virtual environment where PySAM is not importable but pysam is? That would help understand what is happening.

@echedey-ls
Copy link
Contributor

@kandersolar
Windows paths resolution is case insensitive
You may have more luck in a Linux - based OS (MacOS is). I can't test it right now.

@kandersolar
Copy link
Member

I'm not sure that's true for python imports. For example, try import PVLIB on windows and you will get a ModuleNotFoundError.

Regardless, this linux job in our CI from 10 hours ago seems to run the relevant tests with no problems: https://github.com/pvlib/pvlib-python/actions/runs/9414747009/job/25956808532#step:9:54

@cwhanse
Copy link
Member

cwhanse commented Jun 7, 2024

There is a package called "pysam" which isn't NREL's. Maybe that was installed by mistake? What happens if you

import pysam
pysam.AlignmentFile?

@jngarber-st
Copy link
Author

Thank you all for the responses.

As I walked through each of the questions, the issue became clear. The package pysam is the problem. I had both pysam and nrel-pysam installed, not realizing the former was unofficial. I created a new virtual env, installed only nrel-pysam, and it works as expected. Also, the nrel-pysam version installed with the folder name "PySAM". The pysam install folder is named "pysam", but it conflicts with PySAM if also installed.

Using caps in the package name still goes against Python best practices, but I understand that is not PVLib's doing here and certainly not the end of the world.

Also of note, the order of installing pysam and nrel-pysam doesn't matter. If pysam is installed, the import won't work. Uninstalling pysam after accidentally installing it is fine. I'm using pip to manage package installations, and use virtualenv to manage my virtual environments for each project as I run multiple versions of Python on my machine.

@kandersolar
Copy link
Member

Thanks @jngarber-st for following up. I'm going to close this issue as the issue is external to pvlib, but proposals for how pvlib could handle this problematic scenario better are welcome.

@jngarber-st
Copy link
Author

I agree, the issue is external. I think about the only suggestion I could provide is to have nrel-pysam a dependency so it is installed when installing pvlib. Perhaps a small warning of the potential conflict in the documentation for the appropriate functions that rely on that library.

@cwhanse
Copy link
Member

cwhanse commented Jun 14, 2024

a small warning of the potential conflict in the documentation for the appropriate functions that rely on that library.

I agree with this suggestion. It's only fit_cec_sam at this point. PR welcome.

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

No branches or pull requests

4 participants