Skip to content
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

Rz logging dedup2 #454

Open
wants to merge 15 commits into
base: dev
Choose a base branch
from
Open

Rz logging dedup2 #454

wants to merge 15 commits into from

Conversation

rzinke
Copy link
Collaborator

@rzinke rzinke commented Oct 30, 2024

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

@pep8speaks
Copy link

pep8speaks commented Oct 30, 2024

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! 🍻

Comment last updated at 2024-11-07 20:22:19 UTC

@rzinke
Copy link
Collaborator Author

rzinke commented Nov 5, 2024

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

@@ -475,7 +483,7 @@ def main():
# Pass DEM-filename, loaded DEM array, and lat/lon arrays
LOGGER.info('Download/cropping DEM')
demfile, demfile_expanded, lat, lon = \
ARIAtools.util.dem.prep_dem(**dem_dict)
ARIAtools.util.dem.prep_dem(**dem_dict, run_log=run_log)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this line causes the ariaTSsetup.py to die with the error:
TypeError: prep_dem() got an unexpected keyword argument 'run_log'

run_log needs to be added as a variable at the top of the function or removed from here.


class RunLog:
"""
"""
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

populate or remove the docstring triple quotes

"""
def __init__(self, workdir, verbose=None):
"""
"""
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

populate or remove the docstring triple quotes


def load(self):
"""
"""
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

populate or remove the docstring triple quotes


def update(self, atr_name, atr_value):
"""
"""
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

populate or remove the docstring triple quotes


def __update_runtimes__(self):
"""
"""
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

populate or remove the docstring triple quotes


def __update_configs__(self, atr_name, atr_value):
"""
"""
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

populate or remove the docstring triple quotes


def __write_file_list__(self, files):
"""
"""
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

populate or remove the docstring triple quotes


def __write_extracted_files__(self, extracted_files):
"""
"""
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

populate or remove the docstring triple quotes

@alexfore
Copy link
Collaborator

alexfore commented Nov 8, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants