-
-
Notifications
You must be signed in to change notification settings - Fork 409
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.
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
Fixed italicised Units in sdec_plot #1446
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1446 +/- ##
=======================================
Coverage 71.13% 71.13%
=======================================
Files 67 67
Lines 5523 5523
=======================================
Hits 3929 3929
Misses 1594 1594
Continue to review full report at Codecov.
|
e84b0f1
to
e61075c
Compare
@DhruvSondhi thanks for this - can you show the screenshots comparing how plot looks like before & after these changes? |
Hello @jaladh-singhal ... I am afraid that I don't know the code where I can check this plot ... Can you please share a boilerplate test code or please guide me where I can find the same so that I can plot this graph 😄 |
Hello, I found the way to make the plot but I am facing an issue with the quickstart data set & representation. The error seems to be something related to |
Sorry for late reply. I'm actually working on adding a demo notebook for SDEC plot, you can download and use it from here
@DhruvSondhi this seems strange. I hope you're using the latest tardis codebase and using development installation by |
Thank you very much @jaladh-singhal for the reply 😄 ... Yes I do seem so this is a problem and I have opened an issue #1447 ... Please check this issue out :) |
Also, I have found some more issues which are not that important but are present so I will discuss in another issue 😉 |
This comment has been minimized.
This comment has been minimized.
@jaladh-singhal Also, there is a minor change in the notebook from tardis.widgets import SEDecPlotter It should be from tardis.widgets import SDECPlotter in this notebook |
Hello @jaladh-singhal, I got it working at last ... I do suggest a change which is that this line: |
So atlas, @jaladh-singhal these are the screenshots (And yes they are awesome 😉 , thanks to @jamesgillanders) {Fixed this in the commit below |
@DhruvSondhi regarding your suggestions of |
@jamesgillanders @ycamacho do you think we should move it above, i.e. before |
@DhruvSondhi the screenshots look good. @jamesgillanders can you double check that you wanted |
Note: @jamesgillanders don't merge it until #1438 is merged and @DhruvSondhi rebases this PR |
Sure I can do that ... No issues 😄 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See comments in code - the only that this PR should be changing is the italicised units in the x and y axes. Leave the F_lambda and L_lambda as is
tardis/widgets/sdec_plot.py
Outdated
@@ -876,10 +876,14 @@ def generate_plot_mpl( | |||
self.ax.set_xlabel(r"Wavelength $[\AA]$", fontsize=12) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this x-axis unit is still italicised
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I will do that too but the problem didn't seem to occur in the X-axis though I will hard code it that way :)
@@ -876,10 +876,14 @@ def generate_plot_mpl( | |||
self.ax.set_xlabel(r"Wavelength $[\AA]$", fontsize=12) | |||
if distance: # Set y-axis label for flux | |||
self.ax.set_ylabel( | |||
r"$F_{\lambda}$ [erg/s/cm^{2}/$\AA$]", fontsize=12 | |||
r"Flux [erg $\mathrm{s^{-1}}$ $\mathrm{cm^{-2}}$ $\mathrm{\AA^{-1}}$]", | |||
fontsize=12, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There seems to be some confusion about what to change on the axis labels - I didn't help things with a vague comment, but I am not proposing any change to the actual label - just that the Angstrom unit is no longer italicised. The comment posted was just grabbed from one of my programs illustrating how to use mathrm. Can this be reverted back to F_lambda?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, No Issues but the F_lambda & L_lambda will be italicised ... I will update the screenshots with the same :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
self.ax.set_ylabel( | ||
r"Luminosity [erg $\mathrm{s^{-1}}$ $\mathrm{\AA^{-1}}$]", | ||
fontsize=12, | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above - roll back to L_lambda
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, No problems :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jaladh-singhal - I have no strong opinions on where exactly the virtual_packet_logging flag should appear. Whatever you think is most suitable is fine. |
Just pushing changes ... this looks awesome & good to go :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the code modifications fix the flagged issue - happy for it to be merged, pending second review
7fbbd52
to
7947f9a
Compare
Hello @jamesgillanders & @jaladh-singhal , I have rebased this branch upon the updated master ... Please check & confirm the same 😄 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me!
* Fixed italicised Units in sdec_plot * Fixed Italics Luminosity & Flux Lambda representation * Reverted back to old notation
This PR tries to Fix #1445
Description
Changed the labels to as to match the comment as mentioned
Motivation and Context
Fixes #1445
How Has This Been Tested?
Local Changes
Screenshots (if appropriate):
Types of changes
Checklist: