Skip to content

clean up and improve expansion 1 plotting and reporting - #188

Merged
chantelwetzel-noaa merged 6 commits into
mainfrom
expansion--1-plotting
Oct 22, 2026
Merged

chantelwetzel-noaa merged 6 commits into
mainfrom
expansion--1-plotting

Conversation

@chantelwetzel-noaa

Copy link
Copy Markdown
Member

This pull request refactors and improves the codebase for expansion factor calculations and related plotting in several R functions. The main focus is on cleaning up NA and Inf handling, improving plot output and labeling, and making the code and user messaging more robust and clear. Below are the most important changes grouped by theme:

Refactoring NA/Inf Handling and User Messaging:

  • Simplified and clarified how NA values are tracked and reported in getExpansion_1(), removing handling of Inf values and focusing only on NA cases; updated user messages to reflect this, and now track NA replacements in a new replace_na column instead of the old replace_na_inf.
  • Removed verbose NA/Inf reporting and associated plot generation from EF1_Denominator(), streamlining output and focusing on relevant data. [1] [2] [3]

Plotting Improvements:

  • Updated plotting logic in getExpansion_1() to generate clearer boxplots for expansion factors by year and state, separated for lengths and ages, and removed obsolete NA/Inf barplots.
  • Overhauled the plots in EF1_Numerator(): replaced a boxplot of sample weights per trip with new barplots showing the proportion and count of samples with NA values in Trip_Sampled_Lbs by gear, year, and state; improved axis labeling, sizing, and facetting for better readability. [1] [2]

Minor Code and Documentation Updates:

  • Updated a comment in getExpansion_1() to clarify the challenges of expanding WA data due to missing trip weights.
  • Added .positai and .claude to .Rbuildignore to exclude these directories from package builds.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

Default verbose execution fails on an undefined counter, and the new plots can misrepresent missing-weight proportions and age-expansion distributions.

3 open issues
What changed in this PR

This PR refines first-stage expansion diagnostics in the PacFIN processing code, focusing on missing-value reporting and clearer plots.

Changes:

  • Simplifies NA reporting and renames the replacement flag to replace_na.
  • Separates length and age expansion plots and adds numerator missing-weight proportions and counts by gear.
  • Removes redundant denominator diagnostics and excludes local tool directories from builds or version control.
File Description
R/​getExpansion_1.R Updates NA reporting and separates length and age boxplots.
R/​EF1_Numerator.R Adds missing-weight proportion and count plots with gear facets.
R/​EF1_Denominator.R Removes redundant diagnostics and names plotWL() arguments.
.Rbuildignore Excludes .positai and .claude from package builds.
.gitignore Ignores .positai.

Review effort: Balanced

Comment thread R/getExpansion_1.R Outdated
Comment thread R/EF1_Numerator.R
Comment thread R/getExpansion_1.R

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

The age-specific missing-value count uses the length expansion factor, and minor labeling, wording, and messaging issues remain.

1 open issue
2 previously missed issues

In code that hasn't changed since last review

Medium severity Count missing age expansions using Expansion_Factor_1_A

R/​getExpansion_1.R:173

This age-specific count tests Expansion_Factor_1_L, so it counts missing length expansions rather than missing age expansions. If this count is retained for reporting, it should use Expansion_Factor_1_A; otherwise it is misleading dead code.

This issue also appears on line 174 of the same file.

Low severity Correct spelling of challenging

R/​getExpansion_1.R:157

challanging is a spelling error in this newly updated comment; please use challenging.

3 resolved since last review

Review effort: Lite

Comment thread R/EF1_Numerator.R Outdated
chantelwetzel-noaa and others added 2 commits October 22, 2026 13:07
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
@chantelwetzel-noaa
chantelwetzel-noaa merged commit 0ba81e7 into main Oct 22, 2026
8 checks passed
@chantelwetzel-noaa
chantelwetzel-noaa deleted the expansion--1-plotting branch October 22, 2026 20:59
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