Grotrian Diagram Plot Appearance

[AyushiDaksh]

📝 Description

This PR is part of the Grotrian Diagram project, part of Google Summer of Code 2023. Project description and related PRs/Issues can be found on the project page.

This PR aims to discuss and finalize the look and feel of the Grotrian Diagram plot. Discussions/commits related to the appearance of the plot should be placed here.

Type: 🚀 feature

📌 Resources

Project Link: https://github.com/users/AyushiDaksh/projects/1
Idea Link: https://tardis-sn.github.io/gsoc_2023/ideas/
GSoC Project Link: https://summerofcode.withgoogle.com/programs/2023/projects/HxymUMRe

🚦 Testing

How did you test these changes?

• Testing pipeline
• Other method (describe)
• My changes can't be tested (explain why)

☑️ Checklist

• I requested two reviewers for this pull request
• I updated the documentation according to my changes
• I built the documentation by applying the build_docs label

Note: If you are not allowed to perform any of these actions, ping (@) a contributor.

[review-notebook-app]

Check out this pull request on 

See visual diffs & provide feedback on Jupyter Notebooks.

---

Powered by ReviewNB

[tardis-bot]

*beep* *bop*

Hi, human.

I'm the @tardis-bot and couldn't find your records in my database. I think we don't know each other, or you changed your credentials recently.

Please add your name and email to .mailmap in your current branch and push the changes to this pull request.

In case you need to map an existing alias, follow this example.

[AyushiDaksh]

Some comments on the current behaviour:

• The wavelength range for colorbar is from all transitions in the selected ion only. The class also provides API to update this range manually.
• The level width range is from the level populations currently displayed levels in the plot (which depends on the ion selected and the MAX_LEVELS parameter)
• The y-axis is linear with respect to level number. The level energies are still shown on the secondary y-axis (on the right). The ground state (n=0) is always shown.
• The start x-coordinate of each arrow is always 0 (leftmost). The end x-coordinate is directly proportional to the energy difference between the levels { xEnd = (end_energy - start_energy) / (max_energy - min_energy) }. Even though the y-axis is linear in level number, I have used energy difference for the x-coordinate because it prevents overlapping arrows.

[AyushiDaksh]

Although I have marked this ready to review, the code for shell-wise populations/transition depends on changes in MontecarloTransport to track last line interaction shell IDs. These changes make the shell ID information flow more naturally for our use.

Once that PR gets merged to master, then it's just a matter of rebasing this work on master.

Meanwhile, reviewers please review the files tardis/visualization/widgets/grotrian.py, tardis/visualization/widgets/grotrian_mockup.ipynb and tardis/analysis.py (and ignore the other files for now).

[codecov]

Codecov Report

Merging #2313 (1c9c899) into master (c6e752d) will decrease coverage by 1.50%.
The diff coverage is 20.31%.

❗ Current head 1c9c899 differs from pull request most recent head c9921d9. Consider uploading reports for the commit c9921d9 to get more accurate results

@@            Coverage Diff             @@
##           master    #2313      +/-   ##
==========================================
- Coverage   72.02%   70.52%   -1.50%     
==========================================
  Files         137      138       +1     
  Lines       12463    12800     +337     
==========================================
+ Hits         8976     9027      +51     
- Misses       3487     3773     +286     

Impacted Files	Coverage Δ
tardis/energy_input/gamma_ray_grid.py	19.60% <0.00%> (ø)
.../montecarlo/montecarlo_numba/single_packet_loop.py	24.28% <0.00%> (-1.48%)	⬇️
tardis/montecarlo/montecarlo_numba/vpacket.py	20.35% <0.00%> (-0.37%)	⬇️
tardis/visualization/widgets/grotrian.py	0.00% <0.00%> (ø)
tardis/energy_input/GXPacket.py	33.33% <10.00%> (-25.93%)	⬇️
tardis/analysis.py	31.83% <14.63%> (-2.02%)	⬇️
tardis/energy_input/samplers.py	34.09% <34.09%> (ø)
tardis/montecarlo/montecarlo_numba/opacities.py	38.18% <34.54%> (-3.64%)	⬇️
tardis/energy_input/gamma_ray_interactions.py	20.40% <50.00%> (ø)
tardis/io/model/stella.py	97.36% <97.36%> (ø)
... and 4 more

... and 1 file with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[MarkMageeAstro]

Nice work Ayushi! I've left a few comments that I think should be addressed before we merge. Some more points below.

1. My main comment is that there are still some hard coded options, e.g. He, and this causes the diagram to crash when that ion isn't in the model. We need to remove all hard coded things like that and make it so that the user is required to input an ion when making the plot. We can't assume any ion will be present in the model.

2. Running the diagram locally, I also get an issue with the arrows not rendering correctly. A screenshot is attached here.

3. I tried changing the y_scale option between linear and log for He I using e.g.
   diag.y_scale = 'Log'
   diag.display()
   The figure didn't update though. Either that or the changes were very small? It did update for O I. Can you have a look into that?

4. Updating the colour scale using "diag.colorscale = ..." only changes the colour bar, not the arrows. Similarly, if I set e.g. max_wavelength first, before setting the ion, the colours don't take it into account.

5. Some other options I think are currently set up to work with the widgets in the future, but don't work right now. Maybe I'm wrong, but I couldn't see a way to change e.g. max_levels. This functionality should still be in there so one could set it manually.

[AyushiDaksh]

@MarkMageeAstro Thanks for the detailed review! 😄

Here are my responses to some of your comments:

1. My main comment is that there are still some hard coded options, e.g. He

Sure, I'll make it mandatory to set the ion, otherwise diag.diaplay() will just throw an appropriate error message.

2. Running the diagram locally, I also get an issue with the arrows not rendering correctly. A screenshot is attached here.

This is quite weird. I looked into this and it seems that this happens when I run the notebook on VSCode. However, when I start a Jupyter server and run on browser, the arrowheads render as expected. Is this issue happening with you on VSCode (or your IDE) as well?

3. I tried changing the y_scale option between linear and log for He I using e.g.

It is updating, the changes are just very small for He I.

4. Updating the colour scale using "diag.colorscale = ..." only changes the colour bar, not the arrows. Similarly, if I set e.g. max_wavelength first, before setting the ion, the colours don't take it into account.

My bad! I'll check this right away!

5. Some other options I think are currently set up to work with the widgets in the future, but don't work right now.

Yeah I intend to integrate all options with widgets eventually. And yes, you're right, changing the options you mentioned doesn't work right now. I'll make changes so that the user can change these options by settings them directly for now.

[MarkMageeAstro]

@AyushiDaksh, thanks for the quick response.

2. Running the diagram locally, I also get an issue with the arrows not rendering correctly. A screenshot is attached here.

This is quite weird. I looked into this and it seems that this happens when I run the notebook on VSCode. However, when I start a Jupyter server and run on browser, the arrowheads render as expected. Is this issue happening with you on VSCode (or your IDE) as well?

Yes, this is with VSCode, but it's a new issue. Older versions seemed to be fine. I ran previous versions of the code in VSCode and the arrows rendered fine there. You can see in some of the older screenshots I sent to the slack

3. I tried changing the y_scale option between linear and log for He I using e.g.

It is updating, the changes are just very small for He I.

Okay, good, thanks for checking.

[AyushiDaksh]

@MarkMageeAstro

Yes, this is with VSCode, but it's a new issue. Older versions seemed to be fine. I ran previous versions of the code in VSCode and the arrows rendered fine there. You can see in some of the older screenshots I sent to the slack

Hmm, then maybe some of my recent changes caused that, and the issue went unnoticed by me because I was using browser for my Jupyter notebooks. Will check this out.

Similarly, if I set e.g. max_wavelength first, before setting the ion, the colours don't take it into account.

This was intended. Because going forward, the ion selection and wavelength setting will be part of widgets. And I was of the opinion that if the user changes the ion, the wavelength range should reset to default (and the user can set that again if they want).
However, I can change this behavior and persist the wavelength range setting if that's desirable.

[AyushiDaksh]

My main comment is that there are still some hard coded options, e.g. He

This has been fixed. User is required to set the ion explicitly now, otherwise plotting will throw an appropriate error.

Updating the colour scale using "diag.colorscale = ..." only changes the colour bar, not the arrows

This has been fixed.

[AyushiDaksh]

Hi @MarkMageeAstro !
I have fixed and resolved all the issues you mentioned. Please re-review.

[jamesgillanders]

Fantastic work as always @AyushiDaksh! Mark has done a thorough review and you have implemented everything needed to resolve these -- I have added a few minor comments regarding your code comments, but the code itself is done in my opinion!

[jamesgillanders]

This is great work Ayushi! I am happy with all the updates and resolutions to all the comments raised by me and the other reviewers. Approved!

[aoifeboyle]

Great work @AyushiDaksh ! I really like the result!

[MarkMageeAstro]

Thanks for making the changes, @AyushiDaksh. I think you addressed all of the previous comments I had. I do have one more that I found from running the latest version of the code.

Say I make a diagram for an ion that's in the model. If I then try to change it to an ion that isn't in the model, it crashes. That's fine. After that though, if I run diag.display() again the old ion is shown, but the title changes to the new ion (that isn't in the model). I could imagine this could cause some confusion or inconsistencies. Maybe we should add something so that in the event that it crashes (i.e. by selecting an ion that isn't in the model) then it resets/removes all the plot data? Basically you'd be starting over the next time you run it then.

For example:
diag = GrotrianWidget.from_simulation(sim)
diag.set_ion(2, 0) # He I
diag.display()

produces

Running
diag.set_ion(23, 0) # V I (which isn't in the model)
diag.display()

crashes the code, but then running display again produces the sample plot, except for the title.

[AyushiDaksh]

@MarkMageeAstro I have made all such operations atomic now.

If an error is thrown while setting any physical property (like atomic_number, ion_number, shell), an appropriate error message is shown, and the settings revert to the previous settings

[MarkMageeAstro]

Thanks @AyushiDaksh, this fixed my previous comment