-
Notifications
You must be signed in to change notification settings - Fork 2
Improve code style #93
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
Conversation
📝 WalkthroughWalkthroughThis pull request introduces a mechanism for managing external build and style scripts through a plume-scripts repository. The Makefile is refactored to conditionally clone this repository and defer code-style definitions to an external makefile rather than maintaining them inline. The Pre-merge checks and finishing touches✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
🔇 Additional comments (2)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
scripts/experiment-scripts/generate-grt-figures.py (2)
315-352: Updatesave_to_pdfdocstring to match supported figure types anddfsemanticsThe implementation supports
fig_type == "table4"and, in that case, expects a raw DataFrame (no averaging). The docstring currently says:
fig_type: one of'fig6-table3', 'fig7', 'fig8-9'df: “Data averaged over repeated runs (output ofaverage_over_loops)”Both are misleading for the
table4path.Suggest updating just the docstring, for example:
-def save_to_pdf(df: pd.DataFrame, fig_type: str): - """Save a figure/table of the given type to a PDF file. - - Args: - df: Data averaged over repeated runs (output of `average_over_loops`). - fig_type: One of: 'fig6-table3', 'fig7', 'fig8-9'. - """ +def save_to_pdf(df: pd.DataFrame, fig_type: str): + """Save a figure or table of the given type to a PDF file. + + Args: + df: For 'table4', the raw data frame loaded from CSV; for other + figure types, the data averaged over repeated runs + (output of `average_over_loops`). + fig_type: One of: 'fig6-table3', 'fig7', 'fig8-9', 'table4'. + """
23-33: Movempl.use("Agg")before importingmatplotlib.pyplotThe current code calls
mpl.use("Agg")aftermatplotlib.pyplotis imported, which means the backend selection will have no effect. Matplotlib initializes the backend on first import ofpyplot, so callinguse()after that import is ignored. This will cause backend-related errors in headless environments despite the comment's intent.Restructure the imports to set the backend immediately after importing matplotlib:
-import matplotlib as mpl -import matplotlib.figure -import matplotlib.pyplot as plt -import pandas as pd -import seaborn as sns -from matplotlib.backends.backend_pdf import PdfPages - -mpl.use("Agg") # For headless environments (without GUI) +import matplotlib as mpl +mpl.use("Agg") # For headless environments (without GUI) +import matplotlib.figure +import matplotlib.pyplot as plt +import pandas as pd +import seaborn as sns +from matplotlib.backends.backend_pdf import PdfPages
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
.gitignore(1 hunks)Makefile(1 hunks)pyproject.toml(1 hunks)scripts/experiment-scripts/generate-grt-figures.py(6 hunks)
🔇 Additional comments (5)
.gitignore (1)
28-31: Ignore rule for.plume-scripts/is appropriateIgnoring the locally-cloned
.plume-scripts/directory is consistent with the new Makefile behavior and prevents vendored tooling from being committed. LGTM.scripts/experiment-scripts/generate-grt-figures.py (3)
1-21: Updated module docstring matches the script’s responsibilitiesThe clarified top-level docstring (including the note about Table IV’s time budgets and runs-per-fault) aligns with the code and usage described below. No issues here.
53-61:load_datadocstring improvements look goodThe expanded
load_datadocstring clearly documents arguments and return type and matches the implementation (pd.read_csv). No issues.
257-303: Dynamic header/cell construction for Table IV is soundThe new
generate_table_4docstring and the dynamic construction:headers = ["Project", "Time", *list(table_data.columns[2:])] cell_data = [headers, *table_data.to_numpy().tolist()]correctly derive headers from the pivoted DataFrame and keep rows aligned with the header length. This will also adapt automatically if additional
TestSuiteSourcevalues are added in the CSV. LGTM.If you care about a specific column ordering (e.g., GRT, Randoop, EvoSuite), you may want to sort or reindex
table_data.columns[2:]explicitly to avoid surprises from pandas’ default column ordering.pyproject.toml (1)
7-17: Clarify the scope of thetydependency
ty>=0.0.1a31is included in bothdependencies(line 11) and thedevgroup (line 16). Please confirm whethertyis truly required at runtime or if it's only needed for development and tooling—if the latter, remove it from the maindependencieslist to avoid unnecessary runtime overhead.
No description provided.