-
Notifications
You must be signed in to change notification settings - Fork 22
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
Data download for individual tools #488
Conversation
…running each tool
Hello @hover2pi, Thank you for updating !
Comment last updated at 2021-04-27 19:17:24 UTC |
There is exactly a 0% chance the tests pass on this one on my first try. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After initial review, the code changes look good to me. However, importing exoctk
crashes upon testing (see comment on utils.py
).
# EXOCTK_DATA = os.environ.get('EXOCTK_DATA') | ||
# if not EXOCTK_DATA: | ||
# print('WARNING: The $EXOCTK_DATA environment variable is not set. ' | ||
# 'Contamination overlap will not work. Please set the ' | ||
# 'value of this variable to point to the location of the exoctk_data ' | ||
# 'download folder. Users may retreive this folder by clicking the ' | ||
# '"ExoCTK Data Download" button on the ExoCTK website, or by using ' | ||
# 'the exoctk.utils.download_exoctk_data() function.' | ||
# ) | ||
# TRACES_PATH = None | ||
# else: | ||
# TRACES_PATH = os.path.join(EXOCTK_DATA, 'exoctk_contam', 'traces') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we just remove this instead of leaving it in and commented out?
ON_TRAVIS_OR_RTD = HOME_DIR == '/home/travis' or HOME_DIR == '/Users/travis' or HOME_DIR == '/home/docs' | ||
if not ON_TRAVIS_OR_RTD: | ||
if not EXOCTK_DATA: | ||
print( | ||
'WARNING: The $EXOCTK_DATA environment variable is not set. Please set the ' | ||
'value of this variable to point to the location of the exoctk_data ' | ||
'download folder. Users may retreive this folder by clicking the ' | ||
'download folder. Users may retrieve this folder by clicking the ' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice catch 😆 . "I before E except after C"
@@ -27,21 +27,38 @@ | |||
|
|||
# Supported filters | |||
FILTERS = svo.filters() | |||
NON_JWST = [row['Band'] for row in FILTERS if row['Instrument'] not in ['NIRISS', 'NIRCam', 'NIRSpec', 'MIRI']] | |||
NON_JWST = [filt for filt in FILTERS if not filt.startswith('NIRISS') and not filt.startswith('NIRCam') and not filt.startswith('NIRSpec') and not filt.startswith('MIRI')] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hover2pi This line appears to be crashing when trying to import exoctk
:
>>> import exoctk
pandeia not installed. Functionality limited.
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
File "/Users/bourque/Desktop/repositories/exoctk/exoctk/__init__.py", line 10, in <module>
from . import modelgrid
File "/Users/bourque/Desktop/repositories/exoctk/exoctk/modelgrid.py", line 24, in <module>
from . import utils
File "/Users/bourque/Desktop/repositories/exoctk/exoctk/utils.py", line 30, in <module>
NON_JWST = [filt for filt in FILTERS if not filt.startswith('NIRISS') and not filt.startswith('NIRCam') and not filt.startswith('NIRSpec') and not filt.startswith('MIRI')]
File "/Users/bourque/Desktop/repositories/exoctk/exoctk/utils.py", line 30, in <listcomp>
NON_JWST = [filt for filt in FILTERS if not filt.startswith('NIRISS') and not filt.startswith('NIRCam') and not filt.startswith('NIRSpec') and not filt.startswith('MIRI')]
AttributeError: 'Row' object has no attribute 'startswith'
@bourque Ok, I realized I didn't update the env files. Thanks! |
@hover2pi FYI, I pulled you most recent changes and I am still seeing the same error as before:
|
@bourque can you check if it's running |
@hover2pi Ah yes, good point. I updated
Which version of |
Oh yeah. It requires |
Ok, that should do it @bourque . Sorry to make you look at this so many times. |
@hover2pi No worries! I pulled in your changes and did some spot checking with some of the data packages. Its working like a charm! I think this is good to merge now. |
Data download for individual tools
Data download for individual tools
Resolves #386 by allowing ExoCTK data to be downloaded as needed for each tool.