aria-tools / ARIA-tools

Tools for exploiting ARIA standard products
Apache License 2.0
99 stars 36 forks source link

[ariaDownload.py] Code refactoring and adding progress bar #446

Closed ehavazli closed 4 weeks ago

ehavazli commented 1 month ago

Example Run Output ariaDownload.py --bbox "34.6 34.8 -118.1 -117.9" --track 71 --output Download --start 20211214 --end 20220102

/Users/havazli/miniconda3/envs/ARIA-tools/bin/ariaDownload.py:4: DeprecationWarning: pkg_resources is deprecated as an API. See https://setuptools.pypa.io/en/latest/pkg_resources.html
  __import__('pkg_resources').require('ARIAtools==1.2.1')

2024-10-08 13:20:01,158 - ariaDownload.py - INFO - Downloading 6 products...
Downloading: 100%|███████████████████████████████████████████████████████████████████████| 6/6 [06:24<00:00, 64.08s/file]
2024-10-08 13:26:25,668 - ariaDownload.py - INFO - Download complete. Wrote -- 6 -- products to: /Users/havazli/Desktop/ARIA_tests/products
pep8speaks commented 1 month ago

Hello @ehavazli! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! :beers:

Comment last updated at 2024-10-28 19:23:24 UTC
ehavazli commented 1 month ago

@alexfore

Regarding imports: I have removed the vast majority of from X import Y style from the rest of this code, I want to keep it this way with the full namespace used. (explicit > implicit)

Is there a benefit to importing the whole module rather than the needed pieces? Importing the whole module will increase the memory usage.

Regarding logger messages: The asf_search log level suppression should probably be moved to ARIAtools.util.log similar to other items in there. That will be executed at import time and suppress those messages for everything which imports that module.

Moved the line to log.py

Just as a general note to file away, best practice for logger messages is to use lazy formatting to optimize performance (won't format string unless the message will be written). So avoid f-strings and .format() calls in there but use additional arguments and %-style formatting: https://stackoverflow.com/questions/66993387/use-lazy-formatting-in-logging-functions-pylint-error-message https://medium.com/flowe-ita/logging-should-be-lazy-bc6ac9816906

Switched f-string formatted logger messages to lazy formatting.

I know this was already in there and you did not add this, but we should remove this line:

LOGGER.setLevel(logging.DEBUG if self.args.verbose else logging.INFO)

The global log level should only ever be set in the main() function and I should have deleted this before.

Would you like to open a separate PR for this?

alexfore commented 1 month ago

Is there a benefit to importing the whole module rather than the needed pieces? Importing the whole module will increase the memory usage.

Yes, it is more explicit (zen of python) that is enough of a benefit to (nearly) always import the entire module. There will always be some exceptions where it makes sense but IMO not with basic module imports.

The entire module is imported either way so there is no compute time or memory savings in using from X import Y vs import X https://softwareengineering.stackexchange.com/questions/187403/import-module-vs-from-module-import-function

Can the util.log module treat the asf_search log level setter identically as the others (I think it can be)? Then it can be just another item in the list of module names which have the log level set to logging.WARNING.

Finally, I didn't see any commits addressing or discussion of all of the unrelated changes included in the PR.

ehavazli commented 4 weeks ago

I am closing this PR to separate new features and refactoring into different PRs.