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

chore: logging 5 - remove prints from local_modules directory #179

Merged
merged 39 commits into from
Mar 22, 2024

Conversation

hollandjg
Copy link
Member

@hollandjg hollandjg commented Mar 4, 2024

@hollandjg hollandjg marked this pull request as ready for review March 4, 2024 23:39
Copy link
Member

@cpaniaguam cpaniaguam left a comment

Choose a reason for hiding this comment

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

Thanks for your contributions, John! I pointed some issues with updates to commented lines that seem to have been changed programmatically. What to do with them? If they are kept and used later, extra work would be needed to make the messages lazily evaluated. I also noted a number of specific opportunities for improvement.

src/icesat2_tracks/local_modules/JONSWAP_gamma.py Outdated Show resolved Hide resolved
src/icesat2_tracks/local_modules/JONSWAP_gamma.py Outdated Show resolved Hide resolved
src/icesat2_tracks/local_modules/JONSWAP_gamma.py Outdated Show resolved Hide resolved
src/icesat2_tracks/local_modules/JONSWAP_gamma.py Outdated Show resolved Hide resolved
src/icesat2_tracks/local_modules/m_colormanager_ph3.py Outdated Show resolved Hide resolved
src/icesat2_tracks/local_modules/m_spectrum_ph3.py Outdated Show resolved Hide resolved
src/icesat2_tracks/local_modules/m_tools_ph3.py Outdated Show resolved Hide resolved
src/icesat2_tracks/local_modules/m_tools_ph3.py Outdated Show resolved Hide resolved
src/icesat2_tracks/local_modules/m_tools_ph3.py Outdated Show resolved Hide resolved
src/icesat2_tracks/local_modules/m_tools_ph3.py Outdated Show resolved Hide resolved
@hollandjg
Copy link
Member Author

If they are kept and used later, extra work would be needed to make the messages lazily evaluated.

I agree. A lot of these lines are also touched in #131 (and a lot will be removed), so I don't want to make all the corrections just yet. Once we see what the final merge looks like we can revisit here.

I'm also mindful of the extra effort going through and fixing commented out code might add, and I feel like we're close to the point of diminishing returns here. (There's a strong argument that we hit that point two weeks ago when you said this would be too much work – it's been more work than I expected.)

Copy link
Collaborator

@kmilo9999 kmilo9999 left a comment

Choose a reason for hiding this comment

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

Looks good to me

…gger/5-replace-print-logger-local_modules

# Conflicts:
#	src/icesat2waves/local_modules/m_colormanager_ph3.py
hollandjg and others added 7 commits March 13, 2024 16:44
…gger/5-replace-print-logger-local_modules

# Conflicts:
#	src/icesat2waves/local_modules/jonswap_gamma.py
#	src/icesat2waves/local_modules/m_colormanager_ph3.py
#	src/icesat2waves/local_modules/m_general_ph3.py
#	src/icesat2waves/local_modules/m_spectrum_ph3.py
#	src/icesat2waves/local_modules/m_tools_ph3.py
add more context to debug messages

Co-Authored-By: Carlos Paniagua <68481491+cpaniaguam@users.noreply.github.com>
hollandjg and others added 4 commits March 13, 2024 18:15
Co-Authored-By: Carlos Paniagua <68481491+cpaniaguam@users.noreply.github.com>
Co-Authored-By: Carlos Paniagua <68481491+cpaniaguam@users.noreply.github.com>
Co-Authored-By: Carlos Paniagua <68481491+cpaniaguam@users.noreply.github.com>
Co-Authored-By: Carlos Paniagua <68481491+cpaniaguam@users.noreply.github.com>
Copy link
Member

@cpaniaguam cpaniaguam left a comment

Choose a reason for hiding this comment

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

Just two observations.

Comment on lines 1343 to 1346
_logger.debug("self.iter")
self.iter.__dict__
if self.iter2 is not None:
print("self.iter2")
_logger.debug("self.iter2")
Copy link
Member

Choose a reason for hiding this comment

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

Maybe something like _logger.debug("iter2: %s", self.iter2)? I don't think the current version is very useful as the value of the variable is not displayed.

src/icesat2waves/local_modules/m_tools_ph3.py Outdated Show resolved Hide resolved
kmilo9999
kmilo9999 previously approved these changes Mar 18, 2024
Copy link
Collaborator

@kmilo9999 kmilo9999 left a comment

Choose a reason for hiding this comment

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

Looks good to me. There is small change I suggested

hollandjg and others added 4 commits March 18, 2024 18:02
Co-authored-by: Carlos Paniagua <cpaniaguam@gmail.com>
Co-authored-by: Camilo Diaz <k.diaz99@gmail.com>
Co-Authored-By: Carlos Paniagua <68481491+cpaniaguam@users.noreply.github.com>
Copy link
Member

@cpaniaguam cpaniaguam left a comment

Choose a reason for hiding this comment

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

I think this one has a couple broken ones mixing f-strings with %s. I added suggestions with potential fixes.

src/icesat2waves/local_modules/m_general_ph3.py Outdated Show resolved Hide resolved
src/icesat2waves/local_modules/m_general_ph3.py Outdated Show resolved Hide resolved
src/icesat2waves/local_modules/m_general_ph3.py Outdated Show resolved Hide resolved
src/icesat2waves/local_modules/m_tools_ph3.py Outdated Show resolved Hide resolved
src/icesat2waves/local_modules/m_tools_ph3.py Outdated Show resolved Hide resolved
hollandjg and others added 5 commits March 19, 2024 14:42
Co-authored-by: Carlos Paniagua <cpaniaguam@gmail.com>
Co-authored-by: Carlos Paniagua <cpaniaguam@gmail.com>
Co-authored-by: Carlos Paniagua <cpaniaguam@gmail.com>
Co-authored-by: Carlos Paniagua <cpaniaguam@gmail.com>
Co-authored-by: Carlos Paniagua <cpaniaguam@gmail.com>
cpaniaguam
cpaniaguam previously approved these changes Mar 19, 2024
Copy link
Member

@cpaniaguam cpaniaguam left a comment

Choose a reason for hiding this comment

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

Thanks!

Base automatically changed from feat-logger/4-replace-print-logger-config to main March 22, 2024 14:11
@hollandjg hollandjg dismissed stale reviews from cpaniaguam and kmilo9999 March 22, 2024 14:11

The base branch was changed.

@hollandjg hollandjg merged commit b032621 into main Mar 22, 2024
2 checks passed
@hollandjg hollandjg deleted the feat-logger/5-replace-print-logger-local_modules branch March 22, 2024 14:12
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.

Stuff like this can be done with a single call to print but it'd take extra work to add logging.
3 participants