Skip to content
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

DM-34117: Update stats to improve readability #37

Merged
merged 2 commits into from May 16, 2022
Merged

Conversation

sr525
Copy link
Contributor

@sr525 sr525 commented May 3, 2022

No description provided.

@sr525 sr525 changed the title Tickets/DM-34117 Tickets/DM-34117: Update stats to improve readability May 4, 2022
@@ -0,0 +1,46 @@
description: Make coadd plots for QA
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you mean to push this file?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did not, sorry, I lost track of my test pipeline.

lowStats[sourceType] = lowStatsStr

if np.sum(highSn) > 0:
highMags[sourceType] = f"{np.nanmax(catPlot.loc[highSn, magCol]):.2f}"
sortedMags = np.sort(catPlot.loc[highSn, magCol])
x = int((len(sortedMags)/100)*10)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you want to do an integer division with // here? Otherwise, this is going to be identical to int(len(sortedMags)/10)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left it like this to make it more obvious that I was going for 10%, I can tidy it up if you would prefer.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To me, it looked like you were trying something else here. It might be cleaner to just have this line as int(len(sortedMags)/10) or int(0.1*len(sortedMags)) and have a comment that it's 10%, if you prefer to make it more evident.

python/lsst/analysis/drp/scatterPlot.py Outdated Show resolved Hide resolved
python/lsst/analysis/drp/scatterPlot.py Outdated Show resolved Hide resolved
if len(xs) < 2:
medLine, = ax.plot(xs, np.nanmedian(ys), color,
label="Median: {:0.2f}".format(np.nanmedian(ys)), lw=0.8)
label="Median: {:0.3g}".format(np.nanmedian(ys)), lw=0.8)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
label="Median: {:0.3g}".format(np.nanmedian(ys)), lw=0.8)
label=f"Median: {np.nanmedian(ys):0.3g}", lw=0.8)

python/lsst/analysis/drp/scatterPlot.py Show resolved Hide resolved
@sr525 sr525 force-pushed the tickets/DM-34117 branch 2 times, most recently from 87aec6a to f1554a2 Compare May 5, 2022 18:11
@arunkannawadi
Copy link
Member

Rebasing has picked up old commits, it looks like

Copy link
Member

@arunkannawadi arunkannawadi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Many commits are doing much more than what the commit message says - for e.g., "Fix bugs so that the pipelines runs on dc2" in addition to fixing bugs, does string formatting and converting to scientific notation when there's another commit that does the conversion in other places. Some commits are also negative changes from previous commits - for e.g., addition and removal of print("END OF SOURCE TYPE LIST"). It might be easier to get the branch to a working state, squash all commits and split them into smaller pieces.

lowStats[sourceType] = lowStatsStr

if np.sum(highSn) > 0:
highMags[sourceType] = f"{np.nanmax(catPlot.loc[highSn, magCol]):.2f}"
sortedMags = np.sort(catPlot.loc[highSn, magCol])
x = int((len(sortedMags)/100)*10)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To me, it looked like you were trying something else here. It might be cleaner to just have this line as int(len(sortedMags)/10) or int(0.1*len(sortedMags)) and have a comment that it's 10%, if you prefer to make it more evident.

@arunkannawadi arunkannawadi changed the title Tickets/DM-34117: Update stats to improve readability DM-34117: Update stats to improve readability May 10, 2022
@arunkannawadi
Copy link
Member

arunkannawadi commented May 10, 2022

Could you use {.3e} instead of {.3g}. The latter seems to switch to scientific notation only in some cases. For example,

In [1]: a = 0.000123

In [2]: f"a = {a:.3g}"
Out[2]: 'a = 0.000123'

In [3]: f"a = {a:.3e}"
Out[3]: 'a = 1.230e-04'

In [4]: a = 0.0000123

In [5]: f"a = {a:.3g}"
Out[5]: 'a = 1.23e-05'

In [6]: f"a = {a:.3e}"
Out[6]: 'a = 1.230e-05'

The distinction between the two formatting becomes significant if a has an integer part, although that's unlikely to be the case in these plots.

In [7]: a = 456.789

In [8]: f"a = {a:.3g}"
Out[8]: 'a = 457'

In [9]: f"a = {a:.3e}"
Out[9]: 'a = 4.568e+02'

@sr525
Copy link
Contributor Author

sr525 commented May 12, 2022

Could you use {.3e} instead of {.3g}. The latter seems to switch to scientific notation only in some cases. For example,

In [1]: a = 0.000123

In [2]: f"a = {a:.3g}"
Out[2]: 'a = 0.000123'

In [3]: f"a = {a:.3e}"
Out[3]: 'a = 1.230e-04'

In [4]: a = 0.0000123

In [5]: f"a = {a:.3g}"
Out[5]: 'a = 1.23e-05'

In [6]: f"a = {a:.3e}"
Out[6]: 'a = 1.230e-05'

The distinction between the two formatting becomes significant if a has an integer part, although that's unlikely to be the case in these plots.

In [7]: a = 456.789

In [8]: f"a = {a:.3g}"
Out[8]: 'a = 457'

In [9]: f"a = {a:.3e}"
Out[9]: 'a = 4.568e+02'

We actually get integers and decimals with not that many 0s quite often and I find the scientific notation for them very awkward.

@arunkannawadi
Copy link
Member

Fair enough. This should be good to merge after a rebase.

@sr525 sr525 merged commit 7535993 into main May 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants