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

Add automatic data download #932

Merged

Conversation

WilliamJamieson
Copy link
Contributor

This PR makes it so that instead of erroring out when $WEBBPSF_PATH is not set, that instead it attempts to download the data using the link in the documentation to the directory suggested for storing the data by the documentation. Note that no download will occur if the location suggested by the docs is non-empty; this saves users from constantly re-downloading the data. Warnings are issued to the user in the case that $WEBBPSF_PATH is not set and then another warning if a download is attempted.

@pep8speaks
Copy link

pep8speaks commented Nov 19, 2024

Hello @WilliamJamieson, Thank you for updating !

There are no PEP8 issues in this Pull Request.

Comment last updated at 2024-11-22 21:09:18 UTC

Copy link
Collaborator

@BradleySappington BradleySappington left a comment

Choose a reason for hiding this comment

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

Found a small issue, but overall this looks great

webbpsf/utils.py Show resolved Hide resolved
@mperrin
Copy link
Collaborator

mperrin commented Nov 22, 2024

Thanks @WilliamJamieson!

This is a small point but I wanted to suggest we think about a different default path. FYI we are shortly going to be migrating “webbpsf” to “stpsf” to reflect that it’s Webb + Roman both. So perhaps we can preemptively avoid creating directories with the string “webbpsf” hard coded in them.

@WilliamJamieson
Copy link
Contributor Author

Thanks @WilliamJamieson!

This is a small point but I wanted to suggest we think about a different default path. FYI we are shortly going to be migrating “webbpsf” to “stpsf” to reflect that it’s Webb + Roman both. So perhaps we can preemptively avoid creating directories with the string “webbpsf” hard coded in them.

This is fine with me, I only picked that as a default because that's what the docs used. I'm happy to change it to whatever you would like and update the docs to reflect the new default.

@BradleySappington
Copy link
Collaborator

BradleySappington commented Nov 22, 2024

@WilliamJamieson @mperrin, I'm actually okay with it staying at WebbPSF as long as this gets settled and merged in sooner than later. I will be doing plenty of replacements, so keeping it in line with the documentation is probably our best bet. Or if you choose not to use webbpsf, make sure you change the documentation to match. However if you think this will not be in within a week or so please change to something other than webbpsf (and include the doc change).

@WilliamJamieson
Copy link
Contributor Author

@WilliamJamieson @mperrin, I'm actually okay with it staying at WebbPSF as long as this gets settled and merged in sooner than later. I will be doing plenty of replacements, so keeping it in line with the documentation is probably our best bet. Or if you choose not to use webbpsf, make sure you change the documentation to match. However if you think this will not be in within a week or so please change to something other than webbpsf (and include the doc change).

I wonder if it is possible to hard code the URL and the default location and then just have the doc page use that value

@WilliamJamieson
Copy link
Contributor Author

Note that I think it should be possible to have it pull the latest data version when this fails:

webbpsf/webbpsf/utils.py

Lines 214 to 238 in 629d73e

if data_version_min is not None:
# Check if the data in WEBBPSF_PATH meet the minimum data version
version_file_path = path / 'version.txt'
try:
with open(version_file_path) as f:
version_contents = f.read().strip()
# keep only first 3 elements for comparison (allows "0.3.4.dev" or similar)
parts = version_contents.split('.')[:3]
version_tuple = tuple(map(int, parts))
except (IOError, ValueError):
raise EnvironmentError(
f"Couldn't read the version number from {version_file_path}. (Do you need to update the WebbPSF "
'data? See https://webbpsf.readthedocs.io/en/stable/installation.html#data-install '
'for a link to the latest version.)'
f'\n{MISSING_WEBBPSF_DATA_MESSAGE}'
)
if not version_tuple >= data_version_min:
min_ver = '{}.{}.{}'.format(*data_version_min)
raise EnvironmentError(
f'WebbPSF data package has version {version_contents}, but {min_ver} is needed. '
'See https://webbpsf.readthedocs.io/en/stable/installation.html#data-install '
'for a link to the latest version.'
f'\n{MISSING_WEBBPSF_DATA_MESSAGE}'
)
rather than erroring out. But that was beyond the scope of what I needed.

Copy link

codecov bot commented Nov 22, 2024

Codecov Report

Attention: Patch coverage is 95.65217% with 1 line in your changes missing coverage. Please review.

Project coverage is 62.26%. Comparing base (2dc5d45) to head (3db3adf).
Report is 5 commits behind head on develop.

Files with missing lines Patch % Lines
webbpsf/utils.py 95.65% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop     #932      +/-   ##
===========================================
+ Coverage    62.15%   62.26%   +0.10%     
===========================================
  Files           15       15              
  Lines         7016     7038      +22     
===========================================
+ Hits          4361     4382      +21     
- Misses        2655     2656       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.


🚨 Try these New Features:

@BradleySappington BradleySappington merged commit 4c4cbf8 into spacetelescope:develop Nov 22, 2024
10 checks passed
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

Successfully merging this pull request may close these issues.

4 participants