ENH: Add ability to point to a different Space Weather File location#101
ENH: Add ability to point to a different Space Weather File location#101tristandijkstra wants to merge 4 commits into
Conversation
greglucas
left a comment
greglucas
left a comment
There was a problem hiding this comment.
Overall this looks good to me, thanks for the PR! Some minor comments/questions for some use cases.
| # set the global variable to the new path | ||
| globals()["_F107_AP_PATH"] = Path(path) | ||
|
|
||
| # reset the global data | ||
| globals()["_DATA"] = None |
There was a problem hiding this comment.
Rather than reaching into the global dictionary, this is a bit more standard I think.
| # set the global variable to the new path | |
| globals()["_F107_AP_PATH"] = Path(path) | |
| # reset the global data | |
| globals()["_DATA"] = None | |
| # set the global variables that we are updating | |
| global _F107_AP_PATH, _DATA | |
| _F107_AP_PATH = Path(path) | |
| _DATA = None |
There was a problem hiding this comment.
👍 , I've also updated _load_f107_ap_data() which also used the dictionary. It seems globals are generally discouraged, but I think finding an alternative is outside the scope of this PR.
| ) | ||
|
|
||
|
|
||
| def set_space_weather_path(path: str | Path) -> None: |
There was a problem hiding this comment.
Do we need a way of getting back to the default path / unsetting this variable? Probably not, but I figured I would call it out in case you wanted to add a default None as a sentinal meaning get back to the default.
There was a problem hiding this comment.
I've added the option. I can only vaguely imagine a use case, but its easy enough to add and much more compatible with the new naming.
| if custom_file_used and not _F107_AP_PATH.exists(): | ||
| raise FileNotFoundError( | ||
| f"""Custom space weather path has been set but does not exist: | ||
| {_F107_AP_PATH}""" | ||
| ) |
There was a problem hiding this comment.
You already produce this warning earlier when setting the path initially, I don't think we need it here again. Also, the triple-quote should just be single-quote here.
There was a problem hiding this comment.
This is to cover the case where a user has set the path through the environment variable directly. Its now also covered in test_space_weather_env_variable()
|
Hi @greglucas , thanks for the review. I think I've implemented most the comments and solved the merge conflicts. Happy to make further changes as necessary 😄 |
2 participants
Hello!
I've made an attempt to implement #89 , allowing space weather to be set through a
pymsis.set_space_weather_path()function. My main motivation is that celestrak throttles connection for heavy users.I was not sure about bumping the version. I have decided to keep the celestrak download to the default location (inside the package folder) with the thinking that re-downloading might overwrite a custom SW file.