-
Notifications
You must be signed in to change notification settings - Fork 8
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
Move download functions from ginan/scripts/auto_download_PPP to gnssanalysis #11
Conversation
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.
I think this can go in as is - the functions to deprecate can also be removed once we do a bit of an audit of the rest of the codebase to make sure they are not used elsewhere
@@ -306,9 +306,12 @@ def nominal_span_string(span_seconds: float) -> str: | |||
unit = "W" | |||
span_unit_counts = int(span_seconds // gn_const.SEC_IN_WEEK) | |||
elif span_seconds >= gn_const.SEC_IN_WEEK: | |||
if (span_seconds % gn_const.SEC_IN_WEEK) < gn_const.SEC_IN_DAY: | |||
num_weeks = int(span_seconds // gn_const.SEC_IN_WEEK) |
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.
This is a good idea, makes things a lot clearer for the if
statement that comes next
@@ -339,6 +345,7 @@ def get_install_crx2rnx(override=False, verbose=False): | |||
logging.info(f"crx2rnx already present in {_sys.path[0]}") | |||
|
|||
|
|||
# TODO: Deprecate in favour of the contextmanager? |
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.
We'll need to check where this is still used and replace it, but yep, I think we can definitely get rid of this after that
list(executor.map(download_file_from_cddis, files, _repeat(ftp_folder), _repeat(output_folder))) | ||
|
||
|
||
# TODO: Deprecate? Only supports legacy filenames |
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.
Yeah, I think once we have the other functions that allow download_file_from_cddis
to carry out the expected file downloads, we can definitely get rid of this.
Other functions here I mean stuff like filename generation
Download functions in auto_download_PPP have wider use. Proposing to incorporate into gnssanalysis.