add value error to ensure active virtual packet logging in SDECPlotter #1597

yuyizheng1112 · 2021-05-31T14:42:25Z

This PR allows SDECPlotter to raise error if virtual_packet_logging is not set to True.

Motivation and context

Before this change, SDECPlotter can work even when virtual_packet_logging is not active. In this case, an index error is raised by function in matplotlib while calling the function plotter.generate_plot_mlp(), and the output is confusing.
Since SDECPlotter requires virtual_packet_logging, it should ensure the option is set to true.
How has this been tested?

Testing pipeline.
Other.

Examples

Before change:

After change:

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.

codecov · 2021-05-31T14:51:58Z

Codecov Report

Merging #1597 (fc9db95) into master (011f9b7) will decrease coverage by 5.81%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #1597      +/-   ##
==========================================
- Coverage   67.70%   61.88%   -5.82%     
==========================================
  Files          68       62       -6     
  Lines        6091     5733     -358     
==========================================
- Hits         4124     3548     -576     
- Misses       1967     2185     +218

Impacted Files	Coverage Δ
...dis/tardis/montecarlo/montecarlo_numba/r_packet.py	`25.43% <0.00%> (-5.54%)`	⬇️
tardis/tardis/visualization/tools/sdec_plot.py	`9.94% <0.00%> (-4.94%)`	⬇️
tardis/tardis/io/config_reader.py	`82.08% <0.00%> (-4.25%)`	⬇️
tardis/tardis/montecarlo/spectrum.py	`69.23% <0.00%> (-2.47%)`	⬇️
tardis/tardis/plasma/properties/util/macro_atom.py	`29.03% <0.00%> (-2.22%)`	⬇️
tardis/tardis/util/base.py	`75.78% <0.00%> (-1.99%)`	⬇️
tardis/tardis/plasma/properties/nlte.py	`39.21% <0.00%> (-1.74%)`	⬇️
...s/tardis/montecarlo/montecarlo_numba/macro_atom.py	`43.75% <0.00%> (-1.71%)`	⬇️
...is/tardis/plasma/properties/continuum_processes.py	`36.99% <0.00%> (-1.49%)`	⬇️
.../montecarlo/montecarlo_numba/single_packet_loop.py	`27.11% <0.00%> (-1.22%)`	⬇️
... and 50 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 14cf420...fc9db95. Read the comment docs.

jamesgillanders · 2021-06-01T16:20:10Z

I don't understand why virtual_packet_logging needs to be set to True - I thought we left it so that we could make SDEC plots with either real or virtual packets?

jamesgillanders · 2021-06-01T16:20:35Z

@jaladh-singhal do you know why this isn't working?

wkerzendorf · 2021-06-01T21:26:49Z

@yuyizheng1112 can you write tests for the PR?

yuyizheng1112 · 2021-06-02T02:58:56Z

@jamesgillanders When calling generate_plot_mlp() in the case of virtual_packet_logging: False, the function will raise the error above, and I think this can be confusing for people unfamiliar with tardis.

I also think there is no need to set it to true for those who plot real packets only. But the instruction in sdec_plot.ipynb indicates that the option must set to true in order to generate SDEC plot. I did this to consist with the instruction & avoid the error raised by generate_plot_mlp().

Maybe I can put this exception in function generate_plot_mlp instead of when generating plotter. What do you think about it?

jamesgillanders · 2021-06-03T14:47:32Z

So can I clarify something? There should be functionality to allow the SDEC plot to generate with real packets, but that does not currently work. And this PR will print an error message to explain that the real packets mode don't work. Is that right? Then at some point, after this fix has been implemented, will we fix the real packets mode so that it does work?

jamesgillanders · 2021-06-03T14:48:59Z

If all of that is the case, then we will have to come back and remove this fix, so that the code will allow a plot to be generated using real packets. Or am I misunderstanding the problem being addressed here?

yuyizheng1112 · 2021-06-03T15:04:23Z

@jamesgillanders The default value for packets_mode is "virtual" in this function, so in this case, we are plotting using virtual packets. If virtual packet is not loaded in, it will raise the confusing exception above. As for the real mode, it works well no matter the option is active or not.
Does it make sense? :)

jaladh-singhal · 2021-06-03T15:17:56Z

@jaladh-singhal do you know why this isn't working?

@jamesgillanders becuase there are currently no tests for SDEC plot hence test coverage is failing

jaladh-singhal · 2021-06-03T15:47:04Z

@yuyizheng1112 @jamesgillanders I think I understand both of your reasoning. I see an intersecting solution for this problem:

First, inside SDECPlotter.from_simulation, if sim.runner.virt_logging is False then return dictionary with virtual=None, otherwise keep it as it is i.e. virtual=SDECData.from_simulation(sim, "virtual").
Then, in _calculate_plotting_data(), if packets_mode is virtual and data[packets_mode] is None then raise ValueError that "SDECPlotter doesn't have any data for virtual packets population and SDEC plot for the same was requested. Either set virtual_packet_logging: True in your simulation's configuration file to generate SDEC plot for virtual packets population, or pass packets_mode="real" in your function call to generate SDEC plot for real packets population."

This way even when virtual packets logging is not enabled, SDEC plot can still be obtained for real packets mode (what @jamesgillanders is saying) and will only give error when virtual packets mode is requested by user (what @yuyizheng1112 is trying to do).

jaladh-singhal · 2021-06-03T15:50:58Z

But the instruction in sdec_plot.ipynb indicates that the option must set to true in order to generate SDEC plot. I did this to consist with the instruction & avoid the error raised by generate_plot_mlp().

@yuyizheng1112 Good point! The instruction should actually be changed to "The virtual packet logging capability must be active in order to produce SDEC Plot for virtual packets population. Thus ..." and it should be moved right after 5th cell's output.

jaladh-singhal

@yuyizheng1112 can you please change this as per my comments?

review-notebook-app · 2021-06-04T08:21:27Z

Check out this pull request on

See visual diffs & provide feedback on Jupyter Notebooks.

Powered by ReviewNB

yuyizheng1112 · 2021-06-04T08:26:36Z

@jaladh-singhal Sure. I've changed both ValueError and instruction.

github-actions · 2021-06-04T08:27:46Z

jaladh-singhal

Thanks for the changes @yuyizheng1112 - I had a one minor comment.

Also, I hope you've tested this for config files with vpacket logging not enabled - can you post screenshots of that?

jaladh-singhal · 2021-06-17T18:27:14Z

tardis/visualization/tools/sdec_plot.py

@@ -568,6 +576,11 @@ def _calculate_plotting_data(
                "allowed values are 'virtual' or 'real'"
            )

+        if packets_mode == "virtual" and self.data[packets_mode] is None:
+            raise ValueError(
+                'SDECPlotter doesn\'t have any data for virtual packets population and SDEC plot for the same was requested.\nEither set virtual_packet_logging: True in your configuration file to generate SDEC plot with virtual packets, or pass packets_mode="real" in your function call to generate SDEC plot with real packets.'


It's better to break long string to multiple lines following 80 char limit. Also if you use "..." for your string you will not need to escape ' used inside your string - see how L575-L576 does this

tardis-bot · 2021-06-17T21:36:47Z

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.

jaladh-singhal

Excellent!

…ter (tardis-sn#1597) * add value error to ensure active virtual packet logging * move ValueError and edit docs * format * Split long string

andrewfullard requested review from jaladh-singhal and jamesgillanders June 1, 2021 13:24

andrewfullard added the visualization label Jun 1, 2021

jaladh-singhal requested changes Jun 3, 2021

View reviewed changes

yuyizheng1112 requested a review from jaladh-singhal June 4, 2021 08:27

jaladh-singhal requested changes Jun 17, 2021

View reviewed changes

jaladh-singhal requested a review from marxwillia June 17, 2021 21:36

yuyizheng1112 added 4 commits June 21, 2021 17:20

add value error to ensure active virtual packet logging

38f8a07

move ValueError and edit docs

7a59e35

format

6dea9f3

Split long string

fc9db95

yuyizheng1112 force-pushed the valueerror_for_sdecplotter branch from 5b4069c to fc9db95 Compare June 21, 2021 09:23

jaladh-singhal approved these changes Jun 21, 2021

View reviewed changes

andrewfullard requested a review from KevinCawley June 21, 2021 19:10

KevinCawley approved these changes Jun 21, 2021

View reviewed changes

andrewfullard merged commit 1d7ce51 into tardis-sn:master Jun 22, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add value error to ensure active virtual packet logging in SDECPlotter #1597

add value error to ensure active virtual packet logging in SDECPlotter #1597

yuyizheng1112 commented May 31, 2021 •

edited

codecov bot commented May 31, 2021 •

edited

jamesgillanders commented Jun 1, 2021

jamesgillanders commented Jun 1, 2021

wkerzendorf commented Jun 1, 2021

yuyizheng1112 commented Jun 2, 2021

jamesgillanders commented Jun 3, 2021

jamesgillanders commented Jun 3, 2021

yuyizheng1112 commented Jun 3, 2021

jaladh-singhal commented Jun 3, 2021

jaladh-singhal commented Jun 3, 2021

jaladh-singhal commented Jun 3, 2021

jaladh-singhal left a comment

review-notebook-app bot commented Jun 4, 2021

yuyizheng1112 commented Jun 4, 2021

github-actions bot commented Jun 4, 2021

jaladh-singhal left a comment

jaladh-singhal Jun 17, 2021

tardis-bot commented Jun 17, 2021

jaladh-singhal left a comment

add value error to ensure active virtual packet logging in SDECPlotter #1597

add value error to ensure active virtual packet logging in SDECPlotter #1597

Conversation

yuyizheng1112 commented May 31, 2021 • edited

codecov bot commented May 31, 2021 • edited

Codecov Report

jamesgillanders commented Jun 1, 2021

jamesgillanders commented Jun 1, 2021

wkerzendorf commented Jun 1, 2021

yuyizheng1112 commented Jun 2, 2021

jamesgillanders commented Jun 3, 2021

jamesgillanders commented Jun 3, 2021

yuyizheng1112 commented Jun 3, 2021

jaladh-singhal commented Jun 3, 2021

jaladh-singhal commented Jun 3, 2021

jaladh-singhal commented Jun 3, 2021

jaladh-singhal left a comment

Choose a reason for hiding this comment

review-notebook-app bot commented Jun 4, 2021

yuyizheng1112 commented Jun 4, 2021

github-actions bot commented Jun 4, 2021

jaladh-singhal left a comment

Choose a reason for hiding this comment

jaladh-singhal Jun 17, 2021

Choose a reason for hiding this comment

tardis-bot commented Jun 17, 2021

jaladh-singhal left a comment

Choose a reason for hiding this comment

yuyizheng1112 commented May 31, 2021 •

edited

codecov bot commented May 31, 2021 •

edited