ENH: Add ability to point to a different Space Weather File location#101
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 😄 |
greglucas
left a comment
greglucas
left a comment
There was a problem hiding this comment.
Looks good to me, just one minor nit to suggest a new naming.
| _DATA_FNAME: str = "SW-All.csv" | ||
| _F107_AP_URL: str = f"https://celestrak.org/SpaceData/{_DATA_FNAME}" | ||
| _F107_AP_PATH: Path = Path(pymsis.__file__).parent / _DATA_FNAME | ||
| _F107_AP_DEFAULT_PATH: Path = Path(pymsis.__file__).parent / _DATA_FNAME |
There was a problem hiding this comment.
We should change all of these from PATH to FILE now I think to try and be explicit since in a shell you're generally saying PATH=/usr/bin as a location to look for your binaries, and not the explicit file path. So I'd say we go with PYMSIS_SPACE_WEATHER_FILE= for the naming of the env variable too so we know it needs to be the actual file and not the directory containing the file.
There was a problem hiding this comment.
👍 I changed the relevant variable names and the docstrings to match.
greglucas
left a comment
greglucas
left a comment
There was a problem hiding this comment.
Thank you for this nice contribution!
|
Test failures I think are just the flaky tests that I pushed a fix up for recently and this wasn't branched off of. Merging it over those and we can re-evaluate/revert if need be. |
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.