tardis icon indicating copy to clipboard operation
tardis copied to clipboard

Adding Debug Messages to Log the Status of Simulation

Open DhruvSondhi opened this issue 3 years ago • 8 comments

This PR aims to add more debugging messages to the Simulation Logs to display the simulation's flow.

Description

Motivation and context

Screenshots The new messages look something on these lines. Subject to iterative change. image

How has this been tested?

  • [x] Testing pipeline.
  • [x] Other.

Type of change

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

Checklist

  • [x] 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.
  • [x] I have assigned and requested two reviewers for this pull request.

DhruvSondhi avatar Jul 10 '21 06:07 DhruvSondhi

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.

tardis-bot avatar Jul 15 '21 04:07 tardis-bot

There are some commented out sections of code in this PR. That will be taken care of once we have the discussion about what all needs to be debug logs.

DhruvSondhi avatar Jul 16 '21 05:07 DhruvSondhi

Please keep in mind that there is a bug in the current implementation with the logging done inside the config_reader.py. As the configuration is created before calling the logging_state function, the log_state variable comes in effect from the next run of the simulation. This is because the logger for the config_reader is changing the log state after the run of the simulation.

DhruvSondhi avatar Jul 16 '21 05:07 DhruvSondhi

Codecov Report

Merging #1704 (80e8102) into master (f24be9a) will increase coverage by 1.31%. The diff coverage is n/a.

:exclamation: Current head 80e8102 differs from pull request most recent head b6dadc5. Consider uploading reports for the commit b6dadc5 to get more accurate results Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1704      +/-   ##
==========================================
+ Coverage   57.65%   58.97%   +1.31%     
==========================================
  Files          66       66              
  Lines        6809     6864      +55     
==========================================
+ Hits         3926     4048     +122     
+ Misses       2883     2816      -67     
Impacted Files Coverage Δ
.../montecarlo/montecarlo_numba/single_packet_loop.py 27.11% <0.00%> (-2.67%) :arrow_down:
tardis/tardis/simulation/base.py 83.26% <0.00%> (-0.07%) :arrow_down:
tardis/tardis/analysis.py 30.16% <0.00%> (ø)
tardis/tardis/visualization/tools/sdec_plot.py 9.52% <0.00%> (ø)
...rdis/tardis/montecarlo/montecarlo_configuration.py 100.00% <0.00%> (ø)
tardis/tardis/io/atom_data/base.py 90.05% <0.00%> (+0.28%) :arrow_up:
...s/tardis/visualization/widgets/custom_abundance.py 14.42% <0.00%> (+0.39%) :arrow_up:
tardis/tardis/model/density.py 92.95% <0.00%> (+0.42%) :arrow_up:
tardis/tardis/io/config_reader.py 82.77% <0.00%> (+0.68%) :arrow_up:
tardis/tardis/plasma/standard_plasmas.py 73.83% <0.00%> (+0.71%) :arrow_up:
... and 5 more

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 f24be9a...b6dadc5. Read the comment docs.

codecov[bot] avatar Jul 16 '21 05:07 codecov[bot]

As discussed - one PR - one idea . Can you please split up this PR in 3-4 PRs with a maximum of 2-3 files changed

wkerzendorf avatar Jul 21 '21 17:07 wkerzendorf

As discussed - one PR - one idea . Can you please split up this PR in 3-4 PRs with a maximum of 2-3 files changed

Hello Wolfgang, this is not possible as this PR contains all the debug messages that are added wrt to the Flow chart that was created. Creating multiple PRs for the same defeats the purpose of the way the messages were added in order.

DhruvSondhi avatar Aug 25 '21 06:08 DhruvSondhi

@DhruvSondhi What is going on with this PR? @andrewfullard had a couple of comments. I think we could merge after they are fixed.

wkerzendorf avatar Oct 21 '21 09:10 wkerzendorf

Hello @wkerzendorf , I didn't have time to work on this PR due to the tracking functionality that was being developed. I am not completely sure on what all needs to be taken care here but I would do the housekeeping & get it ready for a review. Majority of the debug messages are now in their intended place.

DhruvSondhi avatar Oct 21 '21 10:10 DhruvSondhi

Closing as stale- the work can be reused in the future.

andrewfullard avatar Jun 16 '23 15:06 andrewfullard