gwpy / gwsumm

Gravitational-wave interferometer summary information system
GNU General Public License v3.0
12 stars 23 forks source link

Handle changes in kwarg defaults in Axes.scatter (take 2) #333

Closed duncanmmacleod closed 1 year ago

duncanmmacleod commented 2 years ago

This PR revives #328.

codecov[bot] commented 2 years ago

Codecov Report

Merging #333 (c924008) into master (043bdec) will not change coverage. The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #333   +/-   ##
=======================================
  Coverage   49.52%   49.52%           
=======================================
  Files          57       57           
  Lines        8420     8420           
=======================================
  Hits         4170     4170           
  Misses       4250     4250           
Flag Coverage Δ
Linux 49.52% <ø> (?)
macOS 49.52% <ø> (ø)
python3.10 49.52% <ø> (ø)
python3.7 48.68% <ø> (?)
python3.8 49.52% <ø> (?)
python3.9 49.52% <ø> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
gwsumm/plot/triggers/__main__.py 98.10% <ø> (ø)
gwsumm/plot/triggers/core.py 17.77% <ø> (ø)

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

eagoetz commented 2 years ago

@duncanmmacleod Thanks for the revised PR to make this work. A couple items:

Thanks again!

duncanmmacleod commented 2 years ago

@duncanmmacleod Thanks for the revised PR to make this work. A couple items:

  • The changes look straightforward fixes.
  • Do you have an example to prove this works?

No. I will endeavour to remember how to run my own code to generate one...

  • Can you re-run the CI tests? It looks like a lot of them have failures

Done, green CI across the board!

Thanks again!

eagoetz commented 2 years ago

@duncanmmacleod A gentle nudge on this. Waiting for an example to prove this works, if it is possible to generate. Thanks!

eagoetz commented 2 years ago

Sorry for the ping @duncanmmacleod, I know you were on holiday. Just checking back with you on the status of this and wanted to make sure it didn't fall off your radar. Thanks

robertbruntz commented 2 years ago

@duncanmmacleod A second to Evan's nudge.

duncanmmacleod commented 2 years ago

Gents, I've rebased this again, updating the GWpy requirement to 3.0.0, will try and remember how to generate an example.

eagoetz commented 2 years ago

Gents, I've rebased this again, updating the GWpy requirement to 3.0.0, will try and remember how to generate an example.

Hi @duncanmmacleod any progress on this front? Thank you!

eagoetz commented 1 year ago

@duncanmmacleod I want to try and get the new release of gwsumm out this week, maybe tomorrow. Do you think that you will be able to address this PR this week? If so, that would be really wonderful. Thanks in advance!

eagoetz commented 1 year ago

@duncanmmacleod I'm not sure this is enough to fix the issue. I've copied the changes in this PR and made a summary page for the omicron triggers recently from H1: https://ldas-jobs.ligo-wa.caltech.edu/~evan.goetz/summary/day/20221122/lock/glitches/

In the upper left plot, the triggers with higher SNR are still being obscured by the lower SNR triggers. 😞 Somehow the sorting is still not working properly

Edit: I stand corrected! It actually does work if I add sortbycolor = True to the summary page configuration files. Now that I did that it does work as intended.

robertbruntz commented 1 year ago

Note that in order to plot higher SNR points on top of lower SNR points in VET plots, the updated code requires the config file to include "glitchgram-sortbycolor = True" in the "[DEFAULT]" section (e.g., here), as noted here. (I think regular Summary Pages just use sortbycolor = True.)