Skip to content

Conversation

wholmgren
Copy link
Member

@wholmgren wholmgren commented Feb 28, 2019

https://www.esrl.noaa.gov/gmd/grad/solrad/index.html

  • Closes issue #xxxx
  • I am familiar with the contributing guidelines.
  • Fully tested. Added and/or modified tests to ensure correct behavior for all reasonable inputs. Tests (usually) must pass on the TravisCI and Appveyor testing services.
  • Updates entries to docs/sphinx/source/api.rst for API changes.
  • Adds description and name entries in the appropriate docs/sphinx/source/whatsnew file for all changes.
  • Code quality and style is sufficient. Passes LGTM and SticklerCI checks.
  • New code is fully documented. Includes sphinx/numpydoc compliant docstrings and comments in the code where necessary.
  • Pull request is nearly complete and ready for detailed review.

Needs tests.

@wholmgren wholmgren added enhancement solarfx2 DOE SETO Solar Forecasting 2 / Solar Forecast Arbiter io labels Feb 28, 2019
@wholmgren wholmgren added this to the 0.6.2 milestone Feb 28, 2019

test_dir = os.path.dirname(
os.path.abspath(inspect.getfile(inspect.currentframe())))
testfile = os.path.join(test_dir, '../data/abq19056.dat')
Copy link
Member

Choose a reason for hiding this comment

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

should be testfile = os.path.join(test_dir, 'data', '703165TY.csv') for cross-platform compatibility

Copy link
Member Author

Choose a reason for hiding this comment

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

The path you've specified would be pvlib/test/data/703165TY.csv, but the data is in pvlib/data/abq19056.dat. I am pretty sure that os.path handles the .. specification as needed for the platform. The appveyor builds do not object to this pattern.

Copy link
Member

Choose a reason for hiding this comment

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

Of course. I substituted a file I have to test. Doesn't work on Windows. C:\python\pvlib-dev\pvlib-python\pvlib\../data/abq19056.dat

Copy link
Member Author

Choose a reason for hiding this comment

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

Hm... Appveyor tests run on Windows so I am quite confused.

Copy link
Member

Choose a reason for hiding this comment

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

Hmmm... the string is interpreted correctly when a file operation is performed, so it's OK as coded. I still think my suggestion is an improvement.


import os
import inspect

test_dir = os.path.dirname(
    os.path.abspath(inspect.getfile(inspect.currentframe())))

print(test_dir)

testfile = os.path.join(test_dir, '../data/703165TY.csv')
print(testfile)

with open(testfile) as infile:
    r = infile.readline()
    print(r)

produces

C:\python\pvlib-dev\pvlib-python\pvlib\test
C:\python\pvlib-dev\pvlib-python\pvlib\test\../data/703165TY.csv
703165,"SAND POINT",AK,-9.0,55.317,-160.517,7

Copy link
Member Author

Choose a reason for hiding this comment

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

Is '..' acceptable? testfile = os.path.join(test_dir, '..', 'data', '703165TY.csv')

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

that would maintain the OS-specific separator so the string doesn't have a mix of \ and /

Copy link
Member

Choose a reason for hiding this comment

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

or use os.pardir to avoid the ..

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks. I thought os.path.join was smarter than it was because the tests have never complained about that pattern in the past. Looking forward to the Python 3 only days and using pathlib.

test_dir = os.path.dirname(
os.path.abspath(inspect.getfile(inspect.currentframe())))
testfile = os.path.join(test_dir, '../data/abq19056.dat')
testfile_mad = os.path.join(test_dir, '../data/msn19056.dat')
Copy link
Member

Choose a reason for hiding this comment

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

see above comment

@wholmgren
Copy link
Member Author

some examples from the NOAA ftp realtime folder:

abq19059
msn19059

@cwhanse cwhanse merged commit af355bd into pvlib:master Mar 1, 2019
@wholmgren wholmgren deleted the solrad branch March 1, 2019 02:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement io solarfx2 DOE SETO Solar Forecasting 2 / Solar Forecast Arbiter
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants