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

Gain sel tracker #295

Merged
merged 49 commits into from
Jul 9, 2024
Merged

Gain sel tracker #295

merged 49 commits into from
Jul 9, 2024

Conversation

rcervinoucm
Copy link
Collaborator

Creation of a gain selection webmaker script to track the status of the process

Copy link
Member

@morcuended morcuended left a comment

Choose a reason for hiding this comment

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

For formatting, I recommend installing the pre commit hook that does it automatically when committing changes.
Please, also reuse functions defined somewhere else to avoid duplication.

src/osa/scripts/gainsel_webmaker.py Outdated Show resolved Hide resolved
src/osa/scripts/gainsel_webmaker.py Outdated Show resolved Hide resolved
src/osa/scripts/gainsel_webmaker.py Outdated Show resolved Hide resolved
src/osa/scripts/gainsel_webmaker.py Outdated Show resolved Hide resolved
Copy link
Member

@morcuended morcuended left a comment

Choose a reason for hiding this comment

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

thx @rcervinoucm, I left some more comments

src/osa/scripts/gainsel_webmaker.py Outdated Show resolved Hide resolved
src/osa/scripts/gainsel_webmaker.py Outdated Show resolved Hide resolved
src/osa/scripts/gainsel_webmaker.py Outdated Show resolved Hide resolved
src/osa/scripts/gainsel_webmaker.py Outdated Show resolved Hide resolved
src/osa/scripts/gainsel_webmaker.py Outdated Show resolved Hide resolved
src/osa/scripts/gainsel_webmaker.py Outdated Show resolved Hide resolved
src/osa/scripts/gainsel_webmaker.py Outdated Show resolved Hide resolved
src/osa/scripts/gainsel_webmaker.py Outdated Show resolved Hide resolved
src/osa/scripts/gainsel_webmaker.py Outdated Show resolved Hide resolved
src/osa/scripts/gainsel_webmaker.py Outdated Show resolved Hide resolved
src/osa/scripts/gainsel_webmaker.py Outdated Show resolved Hide resolved
src/osa/scripts/gainsel_webmaker.py Outdated Show resolved Hide resolved
src/osa/scripts/gainsel_webmaker.py Show resolved Hide resolved
src/osa/scripts/gainsel_webmaker.py Outdated Show resolved Hide resolved
src/osa/scripts/gainsel_webmaker.py Outdated Show resolved Hide resolved
src/osa/scripts/gainsel_webmaker.py Outdated Show resolved Hide resolved
src/osa/scripts/gainsel_webmaker.py Outdated Show resolved Hide resolved
src/osa/scripts/gainsel_webmaker.py Outdated Show resolved Hide resolved
@morcuended
Copy link
Member

Please also add small tests to ensure the script at least runs. Something like:

def test_gain_selection_webmaker(
    run_summary,
    merged_run_summary,
    drs4_time_calibration_files,
    systematic_correction_files,
    base_test_dir,
):


    output = sp.run(
        ["gainsel_webmaker", "--test", "-d", "2020-01-17"],
        text=True,
        stdout=sp.PIPE,
        stderr=sp.PIPE,
    )
    assert output.returncode != 0

@morcuended
Copy link
Member

also add script name to the entry points list of pyproject.toml so becomes executable

src/osa/tests/test_gainsel_webmaker.py Outdated Show resolved Hide resolved
src/osa/scripts/gainsel_webmaker.py Outdated Show resolved Hide resolved
src/osa/scripts/gainsel_webmaker.py Outdated Show resolved Hide resolved
src/osa/scripts/gainsel_webmaker.py Outdated Show resolved Hide resolved
src/osa/scripts/gainsel_webmaker.py Outdated Show resolved Hide resolved
src/osa/scripts/gainsel_webmaker.py Outdated Show resolved Hide resolved
src/osa/scripts/tests/test_osa_scripts.py Outdated Show resolved Hide resolved
@morcuended morcuended merged commit 82cb8b2 into main Jul 9, 2024
5 checks passed
@morcuended morcuended deleted the gain_selTracker branch July 9, 2024 11:47
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.

2 participants