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

Fix for gaps in segments causing unfilled pie chart plots #392

Merged
merged 3 commits into from
Jan 25, 2024

Conversation

eagoetz
Copy link
Collaborator

@eagoetz eagoetz commented Jan 18, 2024

This PR addresses a problem that there could be gaps in the segments, thus resulting in pie chart plots that can remain unfilled.

Here is an example for O4a.3: https://ldas-jobs.ligo.caltech.edu/~evan.goetz/summary/gps/1384873218-1389456018/segments/
Here is an example for "O4a.4" Dec 1 2023 - Jan 31 2024 ran on Jan 25 2024: https://ldas-jobs.ligo.caltech.edu/~evan.goetz/summary/gps/1385424018-1390694418/segments/

For reference, this is what the "O4a.4" interval run using the current gwsumm code: https://ldas-jobs.ligo.caltech.edu/~evan.goetz/summary/gps/1385424018-1390694419/segments/

We might consider changing the colors for the single IFO pie wedges in the .ini configuration file since the red pie wedges tend to blend together. Or we can chose a different color for the "Missing segments" wedge

@eagoetz eagoetz added this to the 2.2.1 milestone Jan 18, 2024
@eagoetz eagoetz self-assigned this Jan 18, 2024
Copy link

codecov bot commented Jan 18, 2024

Codecov Report

Attention: 50 lines in your changes are missing coverage. Please review.

Comparison is base (b007282) 50.17% compared to head (e0b11f7) 49.96%.

Files Patch % Lines
gwsumm/plot/segments.py 0.00% 49 Missing ⚠️
gwsumm/segments.py 88.89% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #392      +/-   ##
==========================================
- Coverage   50.17%   49.96%   -0.21%     
==========================================
  Files          60       60              
  Lines        8686     8729      +43     
==========================================
+ Hits         4358     4361       +3     
- Misses       4328     4368      +40     
Flag Coverage Δ
Linux 49.96% <13.79%> (-0.21%) ⬇️
python3.10 49.96% <13.79%> (-0.21%) ⬇️
python3.11 49.96% <13.79%> (-0.21%) ⬇️
python3.9 49.96% <13.79%> (-0.21%) ⬇️

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

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

@eagoetz eagoetz force-pushed the fix-network-missing branch from 5aed97a to b79308f Compare January 19, 2024 00:42
@eagoetz eagoetz force-pushed the fix-network-missing branch from b79308f to a600d6c Compare January 19, 2024 00:45
@eagoetz eagoetz marked this pull request as ready for review January 19, 2024 01:13
@eagoetz eagoetz requested a review from iaraota January 19, 2024 01:13
@eagoetz
Copy link
Collaborator Author

eagoetz commented Jan 19, 2024

@iaraota We should go over this together on a Zoom call soon. I think this bug fix also would fix a problem that some folks have been confused about on the summary page pie plots

@iaraota
Copy link
Collaborator

iaraota commented Jan 22, 2024

@eagoetz thank you for working on this!

I think that removing the blank "future segment" when the run is not complete could potentially lead to increased confusion, as the percentages do not add up to 100%.

Additionally, there's a discrepancy with the O4a.3 percentages, summing to 99.9%. Is this possibly due to a rounding issue?

@eagoetz
Copy link
Collaborator Author

eagoetz commented Jan 22, 2024

@iaraota Thanks for the feedback. Maybe I need some more clarification:

  • What do you mean with the blank future segment in the link you provided to my example. Is it the greyed out part in plot 1 for example? I haven't changed anything with those plots, just modifying what happens to generate the pie plot (4th plot on the page)
  • I presume you mean the percentages listed by the pie plot? I would not rely on those to sum exactly to 100% because of rounding issues you mention. Is this what you are referring to?

@eagoetz
Copy link
Collaborator Author

eagoetz commented Jan 22, 2024

@eagoetz The percentages are calculated using the full span, and they should be calculated using just the time covered so far in the pie chart

gwsumm/plot/segments.py Outdated Show resolved Hide resolved
@eagoetz
Copy link
Collaborator Author

eagoetz commented Jan 22, 2024

There also appears to be a bug in the total time calculation that the DMT-ANALYSIS_READY for H1 in O4a.3 is nearly 100% and yet we're finding a missing segment amount for the network of ~1.5%. This seems excessive and needs to be investigated as a potential bug

 - Better define whether running before, during, or after the span for SegmentPiePlot
 - suptitle of the plot is only over the full span if 'include_future' is provided, otherwise based on before, during, or after the span of interest
 - better handle undefined / unknown times
 - gwsumm.segments.get_segments() now has optional ignore_undefined argument by default False, but if True, as is used in NetworkDutyPiePlot, this prevents an undefined segment time from impacting the network segments
 - gwsumm.segments.get_segments() improved reading segments from global memory and taking intersection (this also fixes an incorrect code comment claiming incorrectly that the & operator is a union)
 - fix a bug that was using an iterating variable
 - some code cleanup, more documentation and comments
@eagoetz eagoetz force-pushed the fix-network-missing branch from f61917a to 0c3d6d7 Compare January 24, 2024 23:20
gwsumm/plot/segments.py Outdated Show resolved Hide resolved
@iaraota
Copy link
Collaborator

iaraota commented Jan 25, 2024

@eagoetz Is the "O4a.4 page" the last version of the code? I ask because the percentages on this page exceed 100%.

@eagoetz
Copy link
Collaborator Author

eagoetz commented Jan 25, 2024

@eagoetz Is the "O4a.4 page" the last version of the code? I ask because the percentages on this page exceed 100%.

Good catch, I'll take a look

@eagoetz
Copy link
Collaborator Author

eagoetz commented Jan 25, 2024

@iaraota The percentage issue has now been addressed. Let me know if you have further feedback. Thanks!

@eagoetz eagoetz force-pushed the fix-network-missing branch from f958144 to e8b796e Compare January 25, 2024 18:58
@eagoetz eagoetz force-pushed the fix-network-missing branch from e8b796e to e0b11f7 Compare January 25, 2024 19:42
@eagoetz eagoetz requested a review from iaraota January 25, 2024 19:44
Copy link
Collaborator

@iaraota iaraota left a comment

Choose a reason for hiding this comment

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

LGTM

@eagoetz eagoetz merged commit 02cb6f2 into gwpy:master Jan 25, 2024
9 of 10 checks passed
@eagoetz eagoetz deleted the fix-network-missing branch January 25, 2024 21:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants