Update atmosphere.py - Add a model for spectral corrections

[Jacc0027]

• [x ] Closes #1278
• [x] I am familiar with the contributing guidelines
• [x] Tests added
• [x] Updates entries to docs/sphinx/source/api.rst for API changes.
• [x] Adds description and name entries in the appropriate "what's new" file in docs/sphinx/source/whatsnew for all changes. Includes link to the GitHub Issue with :issue:`num` or this Pull Request with :pull:`num`. Includes contributor name and/or GitHub username (link with :ghuser:`user`).
• [x] New code is fully documented. Includes numpydoc compliant docstrings, examples, and comments where necessary.
• [x] Pull request is nearly complete and ready for detailed review.
• [ ] Maintainer: Appropriate GitHub Labels and Milestone are assigned to the Pull Request and linked Issue.

https://github.com/pvlib/pvlib-python/issues/1278

AM_AOD_PW_spectral_correction.zip

[kandersolar]

Hi @Jacc0027 -- sorry for the delay if you were waiting for feedback on this PR! It looks great so far. To make a more detailed review easier, could you address the code style issues noted by "Stickler CI" on this page? https://github.com/pvlib/pvlib-python/pull/1296/files

This will also need some tests (see the contributing docs) to check that the code works on all the various python versions and package versions we support. Please reach out if you need input from us to move this PR forward :) And thanks again for the contribution!

[Jacc0027]

Hi @Jacc0027 -- sorry for the delay if you were waiting for feedback on this PR! It looks great so far. To make a more detailed review easier, could you address the code style issues noted by "Stickler CI" on this page? https://github.com/pvlib/pvlib-python/pull/1296/files

This will also need some tests (see the contributing docs) to check that the code works on all the various python versions and package versions we support. Please reach out if you need input from us to move this PR forward :) And thanks again for the contribution!

Hi @kanderso-nrel , thank you for your help.

After a bit of time and testing, I finally managed to adapt the code and pass the Stickler CI test.

It is a pleasure for me to contribute to this wonderful project.

I remain at your disposal for any further issues.

[Jacc0027]

Hi @kanderso-nrel, I reply your messages below.

"Thanks for the perseverance dealing with Stickler :) I think the next step is to add unit tests for this new function. The tests for first_solar_spectral_correction are a good template to start from: https://github.com/pvlib/pvlib-python/blob/master/pvlib/tests/test_atmosphere.py#L88-L149"

Attached is a text document in which I have tried to adapt the code of the new function to the pre-existing first solar tests. I have tried to verify the tests by running them locally according to "https://pvlib-python.readthedocs.io/en/latest/contributing.html" but I have not succeeded. I would appreciate help to manage the execution of these tests. Test spectral corrections_AM_AOD_PW.txt

"Together, the tests should run every line of code in the new function. Any lines of code not covered by the tests will be marked by "Codecov" in the same place that Stickler was commenting."

I have doubts as to whether this will be solved once the above tests have been solved or whether a separate action should be taken. Tthe problem is that codecov is not running on all code and I don't know how to change it.

"Also, can you add an entry in /docs/sphinx/source/api.rst for your new function? Next to first_solar_spectral_correction probably makes sense." It has been already requested.

Sorry for the delay in replying, unfortunately I can't devote myself to research and my work has me quite absorbed. With respect to the doubts, the truth is that I had always programmed in Matlab and this is posing a challenge for me.

thank you again for your time and help.

[Jacc0027]

Please, can you help me solve the import of atmosphere.py? With this I will be able to advance in the development of the tests.

Thank you very much in advance.

[kandersolar]

Hi @Jacc0027 -- I apologize for not replying earlier; somehow I missed your previous message.

For the test file you linked in this comment (https://github.com/pvlib/pvlib-python/pull/1296#issuecomment-932930650), I get an error TypeError: 'tuple' object is not callable. This is because the asi tuple defined on lines 29 to 32 in that file does not have a comma after it. So it just needs a comma to fix it:

    ('asi', np.array(
        [[ 1.0555275 ,  0.87707583,  0.72243772],
         [ 1.11225204,  0.93665901,  0.78487953],
         [ 1.14555295,  0.97084011,  0.81994083]]))    # <-- put a comma here

However I'm not sure what to suggest about that screenshot. That code seems ok to me. What error message is being reported?

the truth is that I had always programmed in Matlab and this is posing a challenge for me.

I think many of us have experienced the same :) For the tests, if pytest is not making any sense, you can just assemble a set of inputs and expected outputs, and I am willing to get it working with pytest. But I don't know what the "right answer" should be so I would need you to provide them.

[Jacc0027]

Hi @kanderso-nrel, I'm glad to read you again.

Hi @Jacc0027 -- I apologize for not replying earlier; somehow I missed your previous message.

Don't worry, as the saying goes, "It's better late than never".

For the test file you linked in this comment (#1296 (comment)), I get an error TypeError: 'tuple' object is not callable. This is because the asi tuple defined on lines 29 to 32 in that file does not have a comma after it. So it just needs a comma to fix it:

    ('asi', np.array(
        [[ 1.0555275 ,  0.87707583,  0.72243772],
         [ 1.11225204,  0.93665901,  0.78487953],
         [ 1.14555295,  0.97084011,  0.81994083]]))    # <-- put a comma here

Thanks for the review.

However I'm not sure what to suggest about that screenshot. That code seems ok to me. What error message is being reported?

What happens is that it doesn't let me import code from other files. Maybe I should change IDE to program in python. I am using Visual Studio Code. Do you have any suggestions based on your experience?

the truth is that I had always programmed in Matlab and this is posing a challenge for me.

I think many of us have experienced the same :) For the tests, if pytest is not making any sense, you can just assemble a set of inputs and expected outputs, and I am willing to get it working with pytest. But I don't know what the "right answer" should be so I would need you to provide them.

Let's see if I can help you with this. I attach a compressed file (ZIP) containing two files. One programmed in python that works independently of PVlib and with which the expected results can be obtained. So, that results can be put in the tests file for each type of module based on AM, AOD and PW values. The second file is in csv format and in it we must write in order, and separated by commas, the values ​​of AM, AOD and PW on which we want to calculate the value of MM through the previous file. I have already included a few examples.

In the meantime, I will research other IDEs that allow me to program in python and let me import data from different scripts. Thank you very much for everything, we keep in touch.

AM-AOD-PW_spectral_correction_data_test.zip

[kandersolar]

What happens is that it doesn't let me import code from other files. Maybe I should change IDE to program in python. I am using Visual Studio Code. Do you have any suggestions based on your experience?

The IDE should not really matter, unless perhaps it is misconfigured somehow. For code like from pvlib import atmosphere to succeed, pvlib needs to be accessible from sys.path. You can achieve that in a few ways:

1. Install your modified version of pvlib using pip install /path/to/your/local/pvlib. This will install pvlib into your site-packages folder, which should be one of the entries in sys.path.
2. Or, you can set the current working directory to be the same as where your local copy of pvlib is.

I hope one of those fixes the import issue, but if it still isn't working, it's okay to use the automatic tests on this page (e.g. "Test_bare_Linux Python38") to see if the tests are working.

Anyway I have some edits in https://github.com/Jacc0027/pvlib-python/pull/2 with the tests based on your file, and a few other edits.

[Jacc0027]

What happens is that it doesn't let me import code from other files. Maybe I should change IDE to program in python. I am using Visual Studio Code. Do you have any suggestions based on your experience?

The IDE should not really matter, unless perhaps it is misconfigured somehow. For code like from pvlib import atmosphere to succeed, pvlib needs to be accessible from sys.path. You can achieve that in a few ways:

1. Install your modified version of pvlib using pip install /path/to/your/local/pvlib. This will install pvlib into your site-packages folder, which should be one of the entries in sys.path.
2. Or, you can set the current working directory to be the same as where your local copy of pvlib is.

Thanks, the truth is that I had tried to configure the path from sys.path to pvlib folder but it still did not work.

I hope one of those fixes the import issue, but if it still isn't working, it's okay to use the automatic tests on this page (e.g. "Test_bare_Linux Python38") to see if the tests are working.

Anyway I have some edits in Jacc0027#2 with the tests based on your file, and a few other edits.

Thanks, I have reviewed and merged the changes into this PR, it has been much more simplified and clear. Then, I see that some tests are not executed. What can we do in order to solve it?

[mikofski]

There are some docs you can find on setting up your python environment using either conda or the built in python venv library, then the trick is to activate that environment and install pvlib in develop mode. Follow the installation instructions in the pvlib docs. Then from any ide you can select that virtual end as your interpreter

[kandersolar]

Then, I see that some tests are not executed. What can we do in order to solve it?

This was caused by an unrelated problem and is solved in #1335. If you merge pvlib/master into your branch it will fix it here too. You can use git commands to do that merge, or you can use a GitHub pull request to do the merge with this link: https://github.com/Jacc0027/pvlib-python/compare/Spectral-corrections...pvlib:master

[Jacc0027]

There are some docs you can find on setting up your python environment using either conda or the built in python venv library, then the trick is to activate that environment and install pvlib in develop mode. Follow the installation instructions in the pvlib docs. Then from any ide you can select that virtual end as your interpreter

Thank you so much @mikofski ! I will inquire about it as soon as possible.

[Jacc0027]

I definitely give up for today. There came a time when I only got one line of code not covered by codecov. I think the reason is that it never occurs the case that "module type" and "Coefficients" are different from "None" in the test file. I have updated the values ​​of the tests and in atmosphere.py so that the values ​​of aod_ref and pw_ref are 0.084 and 1.42 respectively. Without knowing how, I get an error in all the tests and I don't know why.

Can anyone help me to solve it? Thank you very much in advance.

[kandersolar]

I think the reason is that it never occurs the case that "module type" and "Coefficients" are different from "None" in the test file.

For this you can define another test function and put it in test_atmosphere.py with the others, something like this:

def test_AM_AOD_PW_spectral_correction_redundant():
    dummy_coeffs = (0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 1)
    with pytest.raises(TypeError):
        atmosphere.AM_AOD_PW_spectral_correction(1, 1, 1, module_type='cdte', coefficients=dummy_coeffs)

For the failing tests, the immediate problem is that the inputs (AM, PW, AOD) are of length 6, and therefore so is the output MM array. But that length 6 array is then compared with the "expected" array, which has length 5. So either the "expected" arrays are missing an element, or the inputs have an extra element. But it looks like there is still a difference in values in addition to the length difference.

[Jacc0027]

I think the reason is that it never occurs the case that "module type" and "Coefficients" are different from "None" in the test file.

For this you can define another test function and put it in test_atmosphere.py with the others, something like this:

def test_AM_AOD_PW_spectral_correction_redundant():
    dummy_coeffs = (0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 1)
    with pytest.raises(TypeError):
        atmosphere.AM_AOD_PW_spectral_correction(1, 1, 1, module_type='cdte', coefficients=dummy_coeffs)

Magnificent @kanderso-nrel, you are amazing!! It worked the first time!

For the failing tests, the immediate problem is that the inputs (AM, PW, AOD) are of length 6, and therefore so is the output MM array. But that length 6 array is then compared with the "expected" array, which has length 5. So either the "expected" arrays are missing an element, or the inputs have an extra element. But it looks like there is still a difference in values in addition to the length difference.

You were right. I was missing one of the expected output data. I have modified it and now everything is correct.

I am finally understanding how Github and tests work. This is mainly due to the great help received. By the way, if anyone has contacts in the Guinness book of records, tell them that I have achieved the highest ratio of Commits per hour in Github! :)

Now we only have 1 error. "codecov / project / library". After reviewing it, I can see errors in the next file: "test.forecast.py". What can we do in this case?

[cwhanse]

I can see errors in the next file: "test.forecast.py". What can we do in this case?

You can ignore that. Please assign me to review when this PR is ready.

[Jacc0027]

I can see errors in the next file: "test.forecast.py". What can we do in this case?

You can ignore that. Please assign me to review when this PR is ready.

Thanks @cwhanse . I believe that all the steps have already been completed before ending this PR. Also, note that I already requested your review a few days ago when I was stuck and it does not allow me to do it again.

I remain at your disposal in case I had to do something additional.

[Jacc0027]

Mostly fixes typos. Can you add descriptions to the other two arguments and fix the merge conflict in the whatsnew file?

Hi Cliff, I guess I've done what you've requested, but am still receiving a conflict regarding to the following file: docs/sphinx/source/api.rst

and the next message: Only those with write access to this repository can merge pull requests.

can you help me to solve this issue?

Thanks in advance

[cwhanse]

@Jacc0027 the merge conflict doesn't show up here, so I think you need to merge the current main from pvlib to see what's going on.

If you have upstream pointed to pvlib-python/main, then checkout this branch and

git pull upstream main

I hope you'll see a git message that you have a conflicted api.rst file and then can fix it.

[Jacc0027]

@Jacc0027 the merge conflict doesn't show up here, so I think you need to merge the current main from pvlib to see what's going on.

If you have upstream pointed to pvlib-python/main, then checkout this branch and

git pull upstream main

I hope you'll see a git message that you have a conflicted api.rst file and then can fix it.

I noticed that the api.rst file did not exist in the main repository, so I have proposed to delete it and there are no more conflicts in this sense.

[cwhanse]

@Jacc0027 that fixed the conflicts. Looks like some tests need corrections.

[Jacc0027]

@Jacc0027 that fixed the conflicts. Looks like some tests need corrections.

Hi, I see..., they weren't appearing before. I'll take a look. Regards,

[Jacc0027]

@Jacc0027 that fixed the conflicts. Looks like some tests need corrections.

I've proposed a couple of changes within the test file. Let's see if something has been solved.

[kandersolar]

Thanks @Jacc0027 for the effort here! I think I see how to get the tests working; would you mind if I pushed some commits to this PR for that? Then after a bit of documentation I think this PR will be ready.

[Jacc0027]

Thanks @Jacc0027 for the effort here! I think I see how to get the tests working; would you mind if I pushed some commits to this PR for that? Then after a bit of documentation I think this PR will be ready.

Hi @kandersolar , no problem. Go ahead with the solution. Thanks for the support!

[kandersolar]

Looks like things are working! I think these are the last main things, @Jacc0027 can you take a look:

• Fill in the two "TODO" parameter descriptions
• Add an entry in /docs/sphinx/source/reference/airmass_atmospheric.rst for this new function
• Add a new item in /docs/sphinx/source/whatsnew/v0.9.6.rst for this enhancement, and yourself as a contributor at the bottom

[Jacc0027]

Looks like things are working! I think these are the last main things, @Jacc0027 can you take a look:

• Fill in the two "TODO" parameter descriptions
• Add an entry in /docs/sphinx/source/reference/airmass_atmospheric.rst for this new function
• Add a new item in /docs/sphinx/source/whatsnew/v0.9.6.rst for this enhancement, and yourself as a contributor at the bottom

Thanks @kandersolar you did it!. I have done the 3 things you've requested me. Please, let me know if you need something else.

[kandersolar]

Hmm, for some reason this PR is not showing any changes to the reference and whatsnew pages: https://github.com/pvlib/pvlib-python/pull/1296/files

Maybe you only changed them locally but did not include them in a commit?

[Jacc0027]

Hmm, for some reason this PR is not showing any changes to the reference and whatsnew pages: https://github.com/pvlib/pvlib-python/pull/1296/files

Maybe you only changed them locally but did not include them in a commit?

I requested to update both files after modifying them via Github online. After requesting the Commit of the changes, I got an option to create a PR for them. Should I create these new PRs?

[kandersolar]

It would be better to include them in this PR so that all the changes are in one place. If you are editing them with Github online, then I think you can add the changes to this PR by editing the files on the specific git branch associated with this PR (the Spectral-corrections branch in your fork of pvlib). For example this is the link for the "what's new" file on the PR branch: https://github.com/Jacc0027/pvlib-python/blob/Spectral-corrections/docs/sphinx/source/whatsnew/v0.9.6.rst

If you edit that file and go to commit the change, it should give you the option "Commit directly to the Spectral-corrections branch", and then I think the commit will show up in this PR.

[Jacc0027]

It would be better to include them in this PR so that all the changes are in one place. If you are editing them with Github online, then I think you can add the changes to this PR by editing the files on the specific git branch associated with this PR (the Spectral-corrections branch in your fork of pvlib). For example this is the link for the "what's new" file on the PR branch: https://github.com/Jacc0027/pvlib-python/blob/Spectral-corrections/docs/sphinx/source/whatsnew/v0.9.6.rst

If you edit that file and go to commit the change, it should give you the option "Commit directly to the Spectral-corrections branch", and then I think the commit will show up in this PR.

Hi @kandersolar, done. Thank you for the guidelines!