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

add get_bad_bpms #427

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

add get_bad_bpms #427

wants to merge 1 commit into from

Conversation

awegsche
Copy link
Contributor

@awegsche awegsche commented Sep 6, 2023

New Feature

This script collects a list of all BPMs declared as bad by either SVD cleaning or isolation forest and compiles a list with number of occurances for each BPM that has been declared at least 50% (25%) of the time.

Purpose

Every now and then BI asks for a list of BPMs that cause trouble for our measurements so that they can take a closer look during the next (E?YET|L)S . This script is intended to facilitate the process of retrieving such a list.

Notes

This script is just a template, if specific measurements / globbing sounds better than just taking unchecked all the measurements in one (or several) output folder, feel free to adapt if necessary

I am not going to write tests for this. It's a script that's run once every 2-3 years.

@awegsche awegsche self-assigned this Sep 6, 2023
@awegsche awegsche changed the title add get_bpms add get_bad_bpms Sep 6, 2023
Copy link
Member

@fsoubelet fsoubelet left a comment

Choose a reason for hiding this comment

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

Details here and there. I'm open to contributing about them if you would like.

I'm wondering if this should be included in omc3 itself. Maybe this is a good fit for pylhc? Would like @JoschD's opinion.

Also, in hindsight to my review in #pylhc/py, I like the little purpose paragraph you have in this PR. Maybe it could be copy-pasted or adapted there?

1. Make sure that the measurements have the desired cleaning method applied.
If needed rerun the measurements with the GUI.

2. Adapt the `DATES` list at the beginning of this script accordingly
Copy link
Member

Choose a reason for hiding this comment

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

I would rather see this as a script input, either as a complete list of dates or as a low and high date range. If this is part of a package anyway then people are not going to install it in editable mode just to modify hard-coded places.

import pathlib
import tfs

ROOT = pathlib.Path("./Betabeat/")
Copy link
Member

Choose a reason for hiding this comment

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

I would rather make the default ROOT as the current working directory (pathlib.Path.cwd()), so you can just cd wherever and then invoke the script.

OUTFILE = None

def main():
global OUTFILE
Copy link
Member

Choose a reason for hiding this comment

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

I feel like global is not a good look, we can pass arguments.


OUTFILE.close()

def bad_bpms_per_date(date: pathlib.Path, plane: str, beam: str):
Copy link
Member

Choose a reason for hiding this comment

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

Here date: pathlib.Path feels counter-intuitive, maybe folder?

Copy link
Member

Choose a reason for hiding this comment

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

Also as the two inner functions, we're not sending back the bad BPMs per se but rather the files listing BPMs.


measurements = [p for p in meas.iterdir() if p.is_dir()]

def get_bad_bpms(path: pathlib.Path):
Copy link
Member

Choose a reason for hiding this comment

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

This is more get_bad_bpms_file

return m
return None

def get_iforest_bpms(path: pathlib.Path):
Copy link
Member

Choose a reason for hiding this comment

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

Idem here.

measurements_iforest = [p for p in measurements_iforest if p is not None]
return (measurements_svd, measurements_iforest)

def get_bad_bpms_for_beam_and_plane_svd(date: str, plane: str, beam: str, bad_bpms_list):
Copy link
Member

Choose a reason for hiding this comment

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

Feels like bad_bpms_list is not expecting a list

bad_bpms_list[words[0]] = 1
return len(measurements)

def get_bad_bpms_for_beam_and_plane_iforest(date: str, plane: str, beam: str, bad_bpms_list):
Copy link
Member

Choose a reason for hiding this comment

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

Same

@pylhctokens pylhctokens added Type: Feature A (suggetion for a) new feature or enhancement in functionality. and removed Feature labels Sep 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Feature A (suggetion for a) new feature or enhancement in functionality.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants