Convergence plot correction: Use step plots

[sarthak-dv]

📝 Description

Type: 🪲 bugfix

In the convergence plot widget, both T and W are plotted against shell velocity. These plots should present their data as a step function, instead of a scatter line, since the T and W values are constant throughout each individual shell.

closes Issue#2519

@jamesgillanders @MarkMageeAstro Please review. Also can you please apply the build_docs label since the plots in the docs will change?

📌 Resources

🚦 Testing

How did you test these changes?

• Testing pipeline
• Other method: Ran convergence plot to see the plot line shape change
• 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.

[tardis-bot]

*beep* *bop*

Hi, human.

The docs workflow has succeeded ✔️

Click here to see your results.

[sarthak-dv]

@jamesgillanders @MarkMageeAstro Can you please review this fix when you have time?
Also can you please apply the build_docs label since the plots in the docs will change?

Thanks in advance!

[atharva-2001]

hi, can you please put up a screenshot showing the changes you made in the pr description?

[sarthak-dv]

@atharva-2001 Done. Also, can you please put the build_docs label on this PR?

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 67.51%. Comparing base (1718002) to head (e91101e).
Report is 5 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2528      +/-   ##
==========================================
- Coverage   68.18%   67.51%   -0.67%     
==========================================
  Files         168      170       +2     
  Lines       14219    14229      +10     
==========================================
- Hits         9695     9607      -88     
- Misses       4524     4622      +98     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[MarkMageeAstro]

Maybe someone else knows, but is 'hv' or 'vh' correct?

Each shell is defined by an inner and outer velocity, so does the temperature, etc. correspond to the inner or outer boundary? I guess that'll determine which one to use or maybe I'm misunderstanding something

[andrewfullard]

Maybe someone else knows, but is 'hv' or 'vh' correct?

Each shell is defined by an inner and outer velocity, so does the temperature, etc. correspond to the inner or outer boundary? I guess that'll determine which one to use or maybe I'm misunderstanding something

This is actually a rather interesting question about how TARDIS works. Temperature and density are actually defined at the centre of the cell.

[jamesgillanders]

Temperature etc. are constant throughout each shell. The point to consider and to address is how that looks on a step plot that is generated by using e.g., v_inner values. In that case the step profile should not be "mid"

[sarthak-dv]

@jamesgillanders @andrewfullard @MarkMageeAstro

I believe "hv" is correct given that we're considering inner velocities. (Reference for different modes here. "hv" changes value at the current x-coordinate (inner_boundary_i) and stays put until the next x-value (inner_boundary_{i+1} = outer_boundary_i)

If I chose outer velocities, then "vh" would have been correct.

"hvh" would be correct if I chose mid velocities for the shells. (Though this case might be problematic if the shells have varying widths)

[MarkMageeAstro]

Maybe I'm over thinking it or confusing myself (both are very real possibilities). The code has changed quite a bit since I last looked at it, I think.

It looked to me like we're not actually using the inner velocities, I thought it was the outer velocities. Based on what Andrew said about the temperature being defined in the middle, if you print out the velocities and temperatures you might get something like:

Velocity Temperature
...
19000 10000
20000 9000

I would assume that means that for velocities 19000 < v <= 20000, the temperature should be 9000. That's not what's shown here though because there is a step down at the last cell. Again, I could be misunderstanding something though

[sarthak-dv]

@MarkMageeAstro

The plot itself steps down at the correct locations, as seen in the attached image:

The last step down is at the inner boundary of the last shell. However, it isn't extended in a horizontal line because the plot doesn't know where to "end" the line segment i.e. outer boundary of last shell).
To fix this, I'll have to additionally pass the outer boundary of the last shell with the same t_rad i.e. (x=v_outer[-1], y=t_rad[-1])

Let me know your thoughts here.

[MarkMageeAstro]

Okay, I see where the confusion has come now. In your first plot, I assumed the step down was at the highest velocity (i.e. v_outer of the last shell), but it's clear from your latest version that's not the case. It's also more obvious which velocities correspond to the temperatures when you print out the complete boundaries for each cell:

Yes, I think it would be good to make sure the line starts at the inner boundary and extends to the outer boundary of the full model.

[sarthak-dv]

@MarkMageeAstro I have updated my branch with the changes. Now the plot looks like this:

[review-notebook-app]

Check out this pull request on 

See visual diffs & provide feedback on Jupyter Notebooks.

---

Powered by ReviewNB

[sarthak-dv]

Can you also please rerun the checks, since the PR was behaving weirdly in between

[jamesgillanders]

Looks great -- this is exactly what was needed! Happy to merge!

[jamesgillanders]

Approved!

[sarthak-dv]

Thanks James!

@MarkMageeAstro This required another approval before merge. Please let me know if this is good to go.

[andrewfullard]

It looks like the convergence plot tests are failing, and black needs to be run on the code.

[sarthak-dv]

@andrewfullard I've fixed the convergence plot tests and ran Black formatter on the file.

[jamesgillanders]

All good with me!

[sarthak-dv]

@MarkMageeAstro Can you please review this? @jamesgillanders has already approved and it needs two approvals to merge