aria-tools / ARIA-tools

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

Rz logging dedup2 #454

Closed rzinke closed 1 week ago

rzinke commented 1 month ago

Latest updates to prevent or minimize reprocessing for ariaTSsetup and ariaExtract. Logging included.

pep8speaks commented 1 month ago

Hello @rzinke! 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-11-19 02:15:37 UTC
rzinke commented 3 weeks ago

Test using

#!/bin/bash

clear

workdir=test

# Download products
ariaDownload.py -t 64 -b '34.5 35.7 -116.5 -114.2' -s 20180101 -e 20180201 -o download

# Extract products
ariaTSsetup.py -f "products/*.nc" -w $workdir --verbose --log-level debug

# Download more products
ariaDownload.py -t 64 -b '34.5 35.7 -116.5 -114.2' -s 20180101 -e 20180215 -o download

# Extract additional products
ariaTSsetup.py -f "products/*.nc" -w $workdir --verbose --log-level debug

# Run again to see how much faster
ariaTSsetup.py -f "products/*.nc" -w $workdir --verbose --log-level debug

workdir='test-aoi'

# Extract products
ariaTSsetup.py -f "products/*.nc" -b aoi.geojson -w $workdir --verbose --log-level debug

# Extract products again
ariaTSsetup.py -f "products/*.nc" -b aoi.geojson -w $workdir --verbose --log-level debug
alexfore commented 3 weeks ago

These are my comments

IMO should phase out use of verbose and let the log level control this type of messaging to the user. The prints which are guarded by if verbose should probably be LOGGER.debug messages.

Recommend to rename run_logging module to runlog (shorter, module name matches class name i.e. runlog.RunLog).

use full namespace to match existing style in imports, almost always with very few exceptions -- this is why it is important to make short and explicit module and package names as per comment above.

imports in run_logging should roughly follow the isort ordering (i.e. in three blocks: system, third party, our own)

doc strings should be populated with something

Ideally, logger messages should use lazy formatting, not f-strings or % strings. These are formatted even if the logger message is not written, whereas lazy formatted strings are only formatted if the message will be written. Changing it everywhere would be a premature optimization but one should get in the habit of using lazy formatting always going forward.

recommend renaming atr to attr in atr_name, atr_value. This is more pythonic -- see getattr / setattr method names.

rzinke commented 3 days ago

This PR is closed due to conflicts from falling behind the dev branch, and because logging was separated into a separate PR (https://github.com/aria-tools/ARIA-tools/pull/456). It is replaced by https://github.com/aria-tools/ARIA-tools/pull/458.