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.

Sign up for GitHub

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

Jump to bottom

Adding Logging Support for Caught Exceptions #1701

Merged
merged 2 commits into from
Jul 12, 2021
Merged

Adding Logging Support for Caught Exceptions #1701

merged 2 commits into from
Jul 12, 2021

Conversation

DhruvSondhi
Copy link
Contributor

This PR aims to add logging support to caught exceptions.

Description

There are numerous exceptions that are caught but are not visible to the user. This PR aims to add support via logging for these exceptions such that it is visible when logging is turned on. Logs at DEBUG level 😄

Motivation and context

Allows for better explanation for silent errors or exceptions that may happen. Logging them will benefit & make it more descriptive for the user to fix and see what is going on in the simulation.

How has this been tested?

  • Testing pipeline.
  • Other. Local Tests

Type of change

  • Bug fix.
  • New feature.
  • Breaking change.
  • None of the above.

Checklist

  • My change requires a change to the documentation.
    • I have updated the documentation accordingly.
    • (optional) I have built the documentation on my fork following the instructions.
  • I have assigned and requested two reviewers for this pull request.

@tardis-bot
Copy link
Contributor

Before a pull request is accepted, it must meet the following criteria:

  • Is the necessary information provided?
  • Is this a duplicate PR?
    • If a new PR is clearly a duplicate, ask how this PR is different from the original PR?
    • If this PR is about to be merged, close the original PR with a link to this new PR that solved the issue.
  • Does it pass existing tests and are new tests provided if required?
    • The test coverage should not decrease, and for new features should be close to 100%.
  • Is the code tidy?
    • No unnecessary print lines or code comments.

@DhruvSondhi DhruvSondhi requested a review from Rodot- July 8, 2021 15:24
andrewfullard
andrewfullard reviewed Jul 8, 2021
View reviewed changes
tardis/analysis.py Outdated
@@ -2,12 +2,15 @@

import re
import os
import logging
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are there changes in this file? They don't do anything.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wanted to ask about this. Do we need to log exceptions for this part of the source code 🤔

tardis/analysis/opacities.py Outdated
@@ -329,7 +329,7 @@ def _calc_thomson_scattering_opacity(self):
"""Calculate the Thomson scattering opacity for each grid cell

\[
\chi_{\mathrm{Thomson}} = n_{\mathrm{e}} \sigma_{\mathrm{T}}
\chi_{\mathrm{Thomson}} = n_{\mathrm{e}} \sigma_{\mathrm{T}}
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this changed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just for formatting consistency with Docstrings 😄

@codecov
Copy link

codecov bot commented Jul 8, 2021 •
edited
Loading

Codecov Report

Merging #1701 (bfcbdec) into master (7d3899e) will decrease coverage by 0.07%.
The diff coverage is n/a.

❗ Current head bfcbdec differs from pull request most recent head c6482c1. Consider uploading reports for the commit c6482c1 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1701      +/-   ##
==========================================
- Coverage   61.06%   60.99%   -0.08%     
==========================================
  Files          63       63              
  Lines        5820     5845      +25     
==========================================
+ Hits         3554     3565      +11     
- Misses       2266     2280      +14
Impacted Files Coverage Δ
tardis/tardis/io/atom_data/base.py 89.77% <0.00%> (-1.51%) ⬇️
tardis/tardis/util/base.py 74.35% <0.00%> (-1.44%) ⬇️
tardis/tardis/base.py 57.69% <0.00%> (-1.40%) ⬇️
...s/tardis/plasma/properties/radiative_properties.py 77.18% <0.00%> (-0.53%) ⬇️
tardis/tardis/io/util.py 71.67% <0.00%> (-0.52%) ⬇️
tardis/tardis/simulation/base.py 82.88% <0.00%> (-0.45%) ⬇️
tardis/tardis/plasma/properties/nlte.py 38.83% <0.00%> (-0.39%) ⬇️
...dis/tardis/plasma/properties/partition_function.py 91.60% <0.00%> (+0.13%) ⬆️
tardis/tardis/io/parsers/csvy.py 90.47% <0.00%> (+0.73%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7d3899e...c6482c1. Read the comment docs.

@DhruvSondhi DhruvSondhi requested a review from andrewfullard July 8, 2021 16:11
@wkerzendorf
Copy link
Member

I think that is a nice start to things. First of all - in general this PR seems to contain a variety of different fixes and a lot of files. In general, fewer PRs targeted to specific issues is better.
My main other question is there are \n\t in front of essentially all debug statements. why?

@DhruvSondhi
Copy link
Contributor Author

My main other question is there are \n\t in front of essentially all debug statements. why?

Hello @wkerzendorf, This is actually due to how the formatting for the logger has been done. Continuing from #1632, I have added a new line & some tab spaces between the messages such that it is easier to read. If we remove this (\n\t) then the message is on the same line, without any space, making it difficult to read (what is the problem or where is the problem because everything is squashed in a single line with line wrapping, etc problems). In the end, it is just for formatting purposes.
If you think that the formatting is unnecessary, I can revert them as they used to be (formatting wise) 😄

@andrewfullard
Copy link
Contributor

Now that you have a fix for the /n/t, you can implement it here

@DhruvSondhi
Copy link
Contributor Author

I have implemented it in PR #1704. I think we can merge this as it is here & then I can resolve this in that PR 😄

@DhruvSondhi DhruvSondhi force-pushed the log_exceptions branch 2 times, most recently from b62e878 to 0a6eb53 Compare July 12, 2021 15:29
@andrewfullard andrewfullard merged commit d1b0a27 into tardis-sn:master Jul 12, 2021
@DhruvSondhi DhruvSondhi deleted the log_exceptions branch July 12, 2021 18:05
atharva-2001 pushed a commit to atharva-2001/tardis that referenced this pull request Oct 1, 2021
@DhruvSondhi
Adding Logging Support for Caught Exceptions (tardis-sn#1701)
49f4bef 
* Added Logging Support to Debug for Caught Exceptions

* Removed \n\t formatting from Exception Messages
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wkerzendorf wkerzendorf wkerzendorf approved these changes

@andrewfullard andrewfullard andrewfullard approved these changes

@Rodot- Rodot- Awaiting requested review from Rodot-

Assignees
No one assigned
Labels
logging
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@DhruvSondhi @tardis-bot @wkerzendorf @andrewfullard