vivaglint-py Bug Report
=======================
Generated: 2026-03-31
Test suite: tests/ (212 tests against test_survey_1000.csv, test_attributes_1000.csv,
            test_attrition_1000.csv, test_survey_1000_cycle2.csv, test_survey_1000_cycle3.csv)
Result: 203 passed, 9 xfailed (known upstream bug), 0 failures


================================================================================
BUG 1 — CRITICAL: extract_survey_factors() crashes on every call
================================================================================

Function:  vivaglint/analyze.py :: extract_survey_factors()
Severity:  Critical — function is completely unusable
Status:    FIXED (2026-03-31) — TypeError crash resolved; oblimin/quartimin issue
           tracked separately in Bug 23

Description:
  extract_survey_factors() calls factor_analyzer.FactorAnalyzer.fit(), which
  internally calls scikit-learn's check_array() with the argument
  force_all_finite="allow-nan". This argument was renamed to ensure_all_finite
  in scikit-learn 1.6 and the old name was fully removed in scikit-learn 1.6+.
  The environment has scikit-learn 1.8.0 installed, so every call to
  extract_survey_factors() raises:

    TypeError: check_array() got an unexpected keyword argument 'force_all_finite'.
               Did you mean 'ensure_all_finite'?

  Both factor_analyzer 0.4.1 and 0.5.1 (latest) contain this broken call.

Affected line:
  factor_analyzer/factor_analyzer.py:630 (third-party library, not vivaglint code)

Reproduction:
  from vivaglint import read_glint_survey, extract_survey_factors
  survey = read_glint_survey("tests/fixtures/test_survey_1000.csv", emp_id_col="EMP ID")
  extract_survey_factors(survey)   # raises TypeError

Fix applied (vivaglint/_factor_analysis.py  +  vivaglint/analyze.py):
  factor_analyzer has been removed entirely as a dependency.  A self-contained
  EFA implementation (_VivaGlintFA in vivaglint/_factor_analysis.py) replaces
  it using only numpy and scipy, which are already required by the package.

  _VivaGlintFA implements:
  - Extraction: Iterated Principal Axis Factoring (PAF) — numerically
    equivalent to psych::fa(fm="pa") in R.
  - Rotations: varimax (SVD-based Kaiser), equamax (Crawford-Ferguson),
    oblimin (gradient descent, gamma=0.0), quartimin (oblimin special case),
    promax (varimax + power transformation), none.
  - Interface: fit(), get_communalities(), get_eigenvalues(),
    get_factor_variance() — identical to what analyze.py expected from
    FactorAnalyzer, so no other code changed.

  Key design decisions:
  - Communalities are stored pre-rotation (they are a property of extraction,
    not rotation) so get_communalities() always returns values in [0, 1].
  - All matrix inversions in oblique rotations use numpy.linalg.pinv (pseudo-
    inverse) rather than linalg.inv, which handles near-singular transformation
    matrices gracefully without raising LinAlgWarning.
  - pyproject.toml: factor_analyzer>=0.4.0 dependency removed.

  The compatibility shim that was applied in the previous fix has been
  removed — it is no longer needed.


================================================================================
BUG 2 — HIGH: analyze_by_attributes() mutates its GlintSurvey input in-place
================================================================================

Function:  vivaglint/analyze.py :: analyze_by_attributes(), lines 859-861
Severity:  High — silent data corruption; causes hard-to-diagnose downstream failures
Status:    FIXED (2026-03-31) — copy.copy(survey) first applied; deepcopy fix applied
           after third-pass testing revealed metadata was still shared (see Bug 24)

Description:
  When attribute_file is provided, analyze_by_attributes() calls
  join_attributes(survey, attribute_file) without making a copy of the survey
  first. join_attributes() modifies the GlintSurvey object in-place by:
    1. Replacing survey.data with the joined DataFrame
    2. Appending the new column names to survey.metadata["attribute_cols"]

  After analyze_by_attributes() returns, the caller's survey object is
  permanently modified. Any subsequent call that uses the same survey object
  (e.g. aggregate_by_manager, summarize_survey) will see the attribute columns
  (Department, Gender, etc.) treated as question columns, causing:

    TypeError: Could not convert string 'MarketingHR...' to numeric

  This bug does NOT occur when attributes are pre-joined via join_attributes()
  before calling analyze_by_attributes(). It only occurs when the attribute_file
  parameter is used.

Affected lines:
  vivaglint/analyze.py:859-861
    if attribute_file is not None:
        from vivaglint.import_ import join_attributes
        survey = join_attributes(survey, attribute_file, emp_id_col=emp_id_col)

Reproduction:
  from vivaglint import read_glint_survey, analyze_by_attributes, summarize_survey
  survey = read_glint_survey("tests/fixtures/test_survey_1000.csv", emp_id_col="EMP ID")
  # This call mutates `survey` in-place:
  analyze_by_attributes(survey, attribute_file="tests/fixtures/test_attributes_1000.csv",
                        scale_points=5, attribute_cols="Department")
  # Now survey.data contains Department, Gender, etc. as columns.
  # This next call crashes:
  summarize_survey(survey, scale_points=5)   # TypeError

Fix:
  Make a defensive copy of the survey before joining attributes. In analyze.py,
  change the attribute_file block to:

    if attribute_file is not None:
        from vivaglint.import_ import join_attributes
        import copy
        survey = join_attributes(copy.copy(survey), attribute_file,
                                 emp_id_col=emp_id_col)

  Note: copy.copy() is sufficient because join_attributes replaces survey.data
  with a new DataFrame rather than modifying the existing one in-place.

Same pattern in analyze_attrition():
  analyze_attrition() does not call join_attributes() internally, so it does
  not share this bug. However, it does require that attribute columns are
  already present in the survey if attribute_cols is specified — this is
  correct behaviour and is working as intended.


================================================================================
BUG 3 — MEDIUM: analyze_attrition() uses deprecated pandas argument
================================================================================

Function:  vivaglint/analyze.py :: analyze_attrition(), line 1051
Severity:  Medium — works today but will break in a future pandas release
Status:    FIXED (2026-03-31) — infer_datetime_format=True removed; strict parsing is now the default

Description:
  analyze_attrition() parses termination dates with:

    pd.to_datetime(attrition_data[term_date_col], infer_datetime_format=True)

  The infer_datetime_format parameter was deprecated in pandas 2.0 and will be
  removed in a future version. pandas 2.x already issues a UserWarning on every
  call:

    UserWarning: The argument 'infer_datetime_format' is deprecated and will be
    removed in a future version. A strict version of it is now the default.

  The function still works correctly because pandas silently ignores the
  deprecated argument for now, but it will break when pandas removes it.

Affected line:
  vivaglint/analyze.py:1051

Fix:
  Remove the infer_datetime_format=True argument entirely. Since pandas 2.0,
  automatic format inference is the default behaviour, so the argument is not
  needed:

    # Before:
    attrition_data[term_date_col] = pd.to_datetime(
        attrition_data[term_date_col], infer_datetime_format=True
    ).dt.date

    # After:
    attrition_data[term_date_col] = pd.to_datetime(
        attrition_data[term_date_col]
    ).dt.date


================================================================================
BUG 4 — LOW: join_attributes() mutates the input GlintSurvey in-place (by design,
             but undocumented and surprising)
================================================================================

Function:  vivaglint/import_.py :: join_attributes()
Severity:  Low — design choice, but creates silent aliasing surprises
Status:    Documentation gap / design concern; not technically a crash

Description:
  join_attributes() modifies the GlintSurvey object it receives by:
    1. Setting survey.data = joined (the merged DataFrame)
    2. Updating survey.metadata["attribute_cols"]
  It then returns the same object.

  This means:

    enriched = join_attributes(survey, attr_file)
    # enriched IS survey — they are the same object
    assert enriched is survey   # True

  Callers who expect join_attributes() to return a new object and leave the
  original unmodified will be surprised. This is the root cause of Bug 2 above
  (analyze_by_attributes unexpectedly mutating its caller's survey).

  The R equivalent (dplyr::left_join) returns a new object and does not modify
  the input. The Python port diverges from R behaviour here.

  Note: When the input is a plain pd.DataFrame, join_attributes() correctly
  returns a new DataFrame without modifying the input. The in-place mutation
  only applies to GlintSurvey objects.

Recommendation:
  Either:
  a) Document clearly in the docstring that the GlintSurvey is modified in-place
     and the same object is returned, OR
  b) Change the behaviour to match R: make a deep copy and return a new
     GlintSurvey, leaving the original unchanged.
  Option (b) is safer and more consistent with R behaviour.


================================================================================
BUG 5 — CRITICAL: pivot_long() crashes when a question column is missing
================================================================================

Function:  vivaglint/reshape.py :: pivot_long()
Severity:  Critical — DataFrame construction fails with TypeError on any survey
           that is missing one of the four question sub-columns
Status:    FIXED (2026-03-31)

Description:
  When a question column (response, comment, topics, or sensitive flag) is not
  present in survey.data, pivot_long() assigns the scalar pd.NA as the value
  for that key in the dict passed to pd.DataFrame():

    "response": data[row["response_col"]].values
                if row["response_col"] in data.columns
                else pd.NA,           # ← scalar, not an array

  pd.DataFrame() cannot broadcast a scalar pd.NA alongside array-valued keys
  of length N.  The result is:

    ValueError: If using all scalar values, you must pass an index.
                  (or similar depending on pandas version)

  This affects any survey export where a question is missing its comment,
  topics, or flag column — a common occurrence when respondents are not invited
  to leave comments.

Affected lines:
  vivaglint/reshape.py:92-104
    "response":        ... else pd.NA,
    "comment":         ... else pd.NA,
    "comment_topics":  ... else pd.NA,
    "sensitive_flag":  ... else pd.NA,

Reproduction:
  # Drop a comment column from the survey data
  survey.data = survey.data.drop(columns=[survey.metadata["questions"]["comment_col"].iloc[0]])
  from vivaglint import pivot_long
  pivot_long(survey)   # ValueError

Fix:
  Replace the scalar pd.NA fallback with an array of pd.NA of the correct
  length so that all dict values have the same shape:

    n = len(data)
    ...
    "response": data[row["response_col"]].values
                if row["response_col"] in data.columns
                else np.full(n, pd.NA),


================================================================================
BUG 6 — CRITICAL: compare_cycles() sorts cycles lexicographically, producing
                   wrong change scores when there are 10+ cycles
================================================================================

Function:  vivaglint/analyze.py :: compare_cycles(), line 384
Severity:  Critical — silently wrong change calculations; no error is raised
Status:    FIXED (2026-03-31) — integer rank map used for sort instead of string comparison

Description:
  compare_cycles() sorts by ["question", "cycle"] before computing lag-based
  change columns (change_from_previous, pct_change_from_previous,
  glint_score_change_from_previous):

    analyses = analyses.sort_values(["question", "cycle"]).reset_index(drop=True)

  When cycle_names uses the default "Cycle N" pattern (or any string-labelled
  cycles), pandas sorts lexicographically, placing "Cycle 10" before "Cycle 2".
  The lag() difference is then computed between the wrong pair of cycles.

  Example with 3+ cycles using default names:
    Lexicographic order: Cycle 1, Cycle 10, Cycle 2, Cycle 3, ...
    Correct order:       Cycle 1, Cycle 2,  Cycle 3, Cycle 10, ...

  The R equivalent uses factor(cycle, levels=cycle_names) which preserves
  the original insertion order.

Affected line:
  vivaglint/analyze.py:384
    analyses = analyses.sort_values(["question", "cycle"]).reset_index(drop=True)

Reproduction:
  from vivaglint import read_glint_survey, compare_cycles
  s1 = read_glint_survey("tests/fixtures/test_survey_1000.csv", emp_id_col="EMP ID")
  # ... load 10 survey objects s1..s10 ...
  result = compare_cycles(s1, s2, s3, s4, s5, s6, s7, s8, s9, s10, scale_points=5)
  # "Cycle 10" will appear between "Cycle 1" and "Cycle 2" — changes are wrong

Fix:
  Convert the cycle column to a Categorical with the original cycle_names order
  before sorting:

    analyses["cycle"] = pd.Categorical(
        analyses["cycle"], categories=cycle_names, ordered=True
    )
    analyses = analyses.sort_values(["question", "cycle"]).reset_index(drop=True)


================================================================================
BUG 7 — HIGH: join_attributes() type mismatch when survey emp_id is numeric
================================================================================

Function:  vivaglint/import_.py :: join_attributes(), line 295
Severity:  High — silent merge failure (all attribute columns become NaN for
           every row) when the survey's employee ID column is numeric
Status:    FIXED (2026-03-31) — attributes[emp_id_col] coerced to match survey dtype before merge

Description:
  join_attributes() reads the attribute CSV with dtype=str, coercing all
  columns — including the emp_id_col — to string:

    attributes = pd.read_csv(attr_path, dtype=str)

  If the survey DataFrame has a numeric emp_id_col (e.g., int64 or float64,
  which occurs when pandas infers the type during read_glint_survey()), the
  merge on emp_id_col is a string-vs-numeric join.  pandas performs a type-safe
  merge: no rows match, so all attribute columns in the joined DataFrame are
  filled with NaN.  No error or warning is raised.

  The R equivalent (dplyr::left_join) coerces both sides to the same type
  automatically.

Affected lines:
  vivaglint/import_.py:295
    attributes = pd.read_csv(attr_path, dtype=str)
  vivaglint/import_.py:320
    joined = data.merge(attributes, on=emp_id_col, how="left")

Fix:
  After loading the attribute file, coerce the emp_id_col in attributes to
  match the dtype of the corresponding column in data:

    survey_id_dtype = data[emp_id_col].dtype
    attributes[emp_id_col] = attributes[emp_id_col].astype(survey_id_dtype)

  This mirrors dplyr::left_join's coercion behaviour and ensures the merge
  always finds matches regardless of how the survey was loaded.


================================================================================
BUG 8 — HIGH: extract_survey_factors() crashes if clean_data is empty
================================================================================

Function:  vivaglint/analyze.py :: extract_survey_factors()
Severity:  High — unhandled crash (confusing error deep in numpy) rather than
           a clear ValueError
Status:    FIXED (2026-03-31) — explicit guard raises ValueError with clear message when
           fewer than 2 complete rows remain after dropna()

Description:
  extract_survey_factors() drops all rows with any missing value before
  factoring:

    clean_data = response_data.dropna()   # line 649

  If the survey has heavy missing data (or if all respondents skipped at least
  one question), clean_data can be an empty DataFrame (0 rows).  The subsequent
  call:

    fa_temp = _VivaGlintFA(
        n_factors=min(len(questions), clean_data.shape[0] - 1),  # → -1 when 0 rows
        rotation=None,
    )
    fa_temp.fit(clean_data)

  passes n_factors = -1 (or 0 after max(1, ...)), and np.corrcoef() on an
  empty array raises:
    ValueError: zero-size array to reduction operation fmax which has no identity
  (or similar numpy error) with no useful diagnostic message.

Affected lines:
  vivaglint/analyze.py:649 (dropna producing empty), 657 (shape[0] - 1 → -1)

Fix:
  Add an explicit guard immediately after the dropna:

    if clean_data.shape[0] < 2:
        raise ValueError(
            f"extract_survey_factors() requires at least 2 respondents with "
            f"complete responses, but only {clean_data.shape[0]} complete rows "
            f"were found after dropping missing values."
        )


================================================================================
BUG 9 — MEDIUM: analyze_attrition() AttributeError on NaT subtraction
================================================================================

Function:  vivaglint/analyze.py :: analyze_attrition(), line 1057-1060
Severity:  Medium — AttributeError in an edge case rather than a graceful NaN
Status:    NOT A BUG — pd.notna() correctly handles both NaT and None; the original
           condition was already correct. The described scenario (None from silent
           parse failure) cannot occur because pd.to_datetime() raises on parse
           errors rather than silently producing None.

Description:
  Days-to-termination is computed row-wise via apply():

    combined_data["_days_to_term"] = combined_data.apply(
        lambda row: (row[term_date_col] - row["_survey_date"]).days
        if pd.notna(row[term_date_col]) and pd.notna(row["_survey_date"])
        else float("nan"),
        axis=1,
    )

  pd.to_datetime().dt.date returns Python datetime.date objects, not
  pandas Timestamps.  When both dates are present the subtraction yields a
  datetime.timedelta, and .days works correctly.  However, if parsing fails
  silently for a single row and produces None (not NaT), pd.notna(None)
  returns True, causing:

    AttributeError: 'NoneType' object has no attribute 'days'

  Additionally the guard pd.notna() is unreliable for raw Python date objects;
  the explicit None check (row[...] is not None) is safer.

Affected lines:
  vivaglint/analyze.py:1057-1060

Fix:
  Replace the guard condition with an explicit None/NaT check that is robust
  to both Python date and pandas NaT:

    lambda row: (row[term_date_col] - row["_survey_date"]).days
    if (row[term_date_col] is not None and row["_survey_date"] is not None)
    else float("nan"),


================================================================================
BUG 10 — MEDIUM: mean_to_glint_score() ZeroDivisionError when scale_points=1
================================================================================

Function:  vivaglint/utils.py :: mean_to_glint_score(), line 120
Severity:  Medium — ZeroDivisionError instead of ValueError
Status:    FIXED (2026-03-31) — guard added: raises ValueError when scale_points < 2

Description:
  mean_to_glint_score() computes:

    round(((mean_val - 1) / (scale_points - 1)) * 100)

  When scale_points=1, the denominator is 0, raising ZeroDivisionError.
  The docstring states scale_points is 2-11, but there is no validation guard
  in the function itself.  A caller passing scale_points=1 gets a confusing
  ZeroDivisionError rather than a clear ValueError.

  Note: get_favorability_map() does validate scale_points (raises ValueError
  for values outside 2-11), so the gap is only in mean_to_glint_score().

Affected line:
  vivaglint/utils.py:120
    return round(((mean_val - 1) / (scale_points - 1)) * 100)

Fix:
  Add a guard at the top of mean_to_glint_score():

    if scale_points < 2:
        raise ValueError(
            f"scale_points must be >= 2, got {scale_points!r}"
        )


================================================================================
BUG 11 — MEDIUM: extract_survey_factors() n_factors can be 0 when clean_data
                  has exactly 1 complete row
================================================================================

Function:  vivaglint/analyze.py :: extract_survey_factors()
Severity:  Medium — degenerate case produces misleading output rather than
           a clear error (related to Bug 8 but distinct; 0-factor model)
Status:    RESOLVED by Bug 8 fix — guard requires >= 2 complete rows, which prevents
           n_factors from ever reaching 0. No additional fix needed.

Description:
  When clean_data has exactly 1 complete row, clean_data.shape[0] - 1 = 0:

    n_factors=min(len(questions), clean_data.shape[0] - 1),  # → 0

  _VivaGlintFA.__init__ stores n_factors=0 and _extract_pa() calls
  max(1, min(0, p)) = 1, so extraction proceeds with 1 factor.  But when
  n_factors is later passed to the second _VivaGlintFA for the actual user-
  requested fit, the Kaiser criterion path can also produce n_factors=0
  because np.sum(ev > 1) can be 0 when all eigenvalues of a single-row
  correlation matrix are ≤ 1, and max(1, 0) = 1 saves it — but the result
  is a degenerate 1-factor solution with no diagnostic to the user.

  The root fix is Bug 8's guard (reject < 2 complete rows). This bug tracks
  the remaining edge case if that guard is applied only to the empty case.

Affected line:
  vivaglint/analyze.py:657
    n_factors=min(len(questions), clean_data.shape[0] - 1),

Fix:
  This is resolved by applying the Bug 8 guard (>= 2 complete rows required).
  No additional fix needed beyond Bug 8.


================================================================================
BUG 12 — MEDIUM: analyze_by_attributes() produces SettingWithCopyWarning
================================================================================

Function:  vivaglint/analyze.py :: analyze_by_attributes()
Severity:  Medium — FutureWarning in pandas 2.x; will silently not update
           in a future pandas release (pandas 3.x Copy-on-Write)
Status:    FIXED (2026-03-31) — slim_df now created via survey_data.loc[mask, cols].copy()

Description:
  Inside the per-group loop, group_data is created as a boolean-indexed view
  of survey_data:

    group_data = survey_data[mask]           # view, not a copy
    slim_df = group_data[available_standard + available_question]

  pandas 2.x issues SettingWithCopyWarning if any code later tries to modify
  slim_df (it is a view of a view).  Under pandas 3.0 Copy-on-Write, this
  pattern becomes silently wrong: modifications to slim_df would not propagate.
  Currently no modification is attempted, so the warning is suppressed, but
  the pattern is fragile.

Affected line:
  vivaglint/analyze.py:892-897
    group_data = survey_data[mask]
    slim_df = group_data[available_standard + available_question]

Fix:
  Add .copy() when creating slim_df to make the intent explicit and future-proof:

    slim_df = survey_data.loc[mask, available_standard + available_question].copy()


================================================================================
BUG 13 — LOW: compare_cycles() reports inf for pct_change when previous mean=0
================================================================================

Function:  vivaglint/analyze.py :: compare_cycles(), lines 386-390
Severity:  Low — undocumented edge case; inf values silently propagate
Status:    Open

Description:
  Percentage change is calculated as:

    analyses["pct_change_from_previous"] = (
        analyses.groupby("question")["mean"].diff()
        / analyses.groupby("question")["mean"].shift()
    ) * 100

  When the previous cycle mean is 0.0, division by zero produces inf (or -inf).
  This is mathematically correct but undocumented.  If a downstream caller
  or report template does not handle inf, it can cause silent cascading errors.

Affected lines:
  vivaglint/analyze.py:386-390

Fix:
  Either document the inf behaviour in the docstring and let callers handle it,
  or replace inf with NaN after the calculation:

    analyses["pct_change_from_previous"] = analyses["pct_change_from_previous"].replace(
        [float("inf"), float("-inf")], float("nan")
    )


================================================================================
BUG 14 — LOW: hierarchy.py silently excludes rows with NaN manager IDs
================================================================================

Function:  vivaglint/hierarchy.py :: get_all_reports() / aggregate_by_manager()
Severity:  Low — working as intended for most surveys, but undocumented;
           top-level employees (no manager) are silently invisible as reports
Status:    Open / Documentation gap

Description:
  _get_all_reports_impl() collects direct reports by filtering:

    data[data[manager_id_col] == manager_id]

  Rows where manager_id_col is NaN are never equal to any manager_id, so
  employees with no manager entry (top-level executives, orphaned records)
  are silently excluded from all roll-up counts in aggregate_by_manager().
  This matches R behaviour but is not documented.

Affected function:
  vivaglint/hierarchy.py :: _get_all_reports_impl()

Fix:
  Document in the docstring that NaN manager IDs are excluded (employees
  with no manager entry do not appear as anyone's reports).  No code change
  needed, but add a logger.debug() note when NaN manager IDs are encountered
  to aid diagnostics.


================================================================================
BUG 15 — LOW: search_comments() empty-result dtype inconsistency
================================================================================

Function:  vivaglint/analyze.py :: search_comments(), line 1263-1265
Severity:  Low — minor API inconsistency; unlikely to cause downstream errors
Status:    Open

Description:
  search_comments() initialises its empty sentinel DataFrame with:

    _empty = pd.DataFrame(
        columns=["question", "response", "comment", "topics"]
    ).astype({"response": "Float64", "comment": str, "topics": str})

  When results are found, the "response" column is populated from the raw survey
  data, which is dtype float64 (lowercase, numpy float).  The empty sentinel
  uses the pandas nullable Float64.  A caller checking the dtype of "response"
  in the returned DataFrame will see different types depending on whether any
  rows were found.  This breaks strict dtype-based checks and can confuse
  pandas-profiling or type-validation tools.

Affected line:
  vivaglint/analyze.py:1263-1265

Fix:
  Either:
  a) Coerce the response column in the non-empty result to Float64 to match
     the sentinel, OR
  b) Change the sentinel to use float64 (numpy) to match the live data.
  Option (b) is lower risk:

    _empty = pd.DataFrame(
        columns=["question", "response", "comment", "topics"]
    ).astype({"response": float, "comment": str, "topics": str})


================================================================================
BUG 16 — LOW: split_survey_data() verbose manual column deduplication
================================================================================

Function:  vivaglint/reshape.py :: split_survey_data()
Severity:  Low — code quality / maintainability concern; no functional bug
Status:    Open / Code smell

Description:
  split_survey_data() manually reconstructs the qualitative and quantitative
  column lists using multiple separate list comprehensions rather than using
  set operations or pandas' built-in column filtering.  As the number of
  standard columns grows, the manual deduplication logic becomes increasingly
  fragile and error-prone.  This is not a crash, but it is a maintenance
  liability.

Affected function:
  vivaglint/reshape.py :: split_survey_data()

Recommendation:
  Refactor the column-splitting logic to use set operations for clarity and
  robustness.  No behaviour change is required.


================================================================================
BUG 17 — LOW: analyze_by_attributes() scale_points type mismatch raises
              TypeError instead of a clear ValueError
================================================================================

Function:  vivaglint/analyze.py :: analyze_by_attributes()
Severity:  Low — confusing error message for callers who pass scale_points
           as a string (e.g. from argparse or a config file)
Status:    Open

Description:
  analyze_by_attributes() passes scale_points directly to summarize_survey(),
  which passes it to get_favorability_map(), which does:

    if scale_points not in _FAVORABILITY_MAP:

  If scale_points is passed as a string (e.g., "5"), this condition is True
  (strings are never keys of _FAVORABILITY_MAP), and the resulting error is:

    ValueError: scale_points must be an integer between 2 and 11, got '5'

  This is actually acceptable.  However, if scale_points is a float (e.g. 5.0),
  the error is:
    ValueError: scale_points must be an integer between 2 and 11, got 5.0

  This is also acceptable, but callers coming from numeric APIs (numpy int64,
  float32, etc.) may find the error confusing because 5.0 is functionally 5.
  A silent int() coercion in the public-facing functions (summarize_survey,
  analyze_by_attributes) would be more ergonomic.

Affected functions:
  vivaglint/analyze.py :: analyze_by_attributes(), summarize_survey()

Recommendation:
  Add scale_points = int(scale_points) near the top of each public function
  that accepts it, with a TypeError guard for non-numeric inputs.  This matches
  R's implicit integer coercion.


================================================================================
BUG 18 — LOW: pivot_long() index alignment issue on filtered data
================================================================================

Function:  vivaglint/reshape.py :: pivot_long()
Severity:  Low — potential index mismatch producing NaN rows when input data
           has a non-default integer index
Status:    Open

Description:
  pivot_long() builds per-question dicts using .values (numpy arrays), which
  strips the index.  However, when the survey DataFrame has a non-contiguous
  or non-zero-based index (e.g. after boolean filtering outside pivot_long),
  mixing .values arrays with index-aligned operations later in the function
  could produce subtle NaN rows.

  Currently all callers pass an unfiltered survey, so this is latent.  If a
  caller passes a filtered slice (e.g. survey.data = survey.data[survey.data["Status"] == "Completed"]),
  the resulting long DataFrame will have correct values but any index-sensitive
  operation downstream may see phantom NaNs.

Affected function:
  vivaglint/reshape.py :: pivot_long()

Fix:
  Reset the index of the input DataFrame at the start of pivot_long():

    data = data.reset_index(drop=True)


================================================================================
BUG 19 — MEDIUM: get_response_dist() and get_correlations() cannot be called
                  with plain DataFrames despite their docstrings claiming support
================================================================================

Functions: vivaglint/analyze.py :: get_response_dist(), get_correlations()
Severity:  Medium — API contract broken; plain DataFrame callers get an unhelpful
           internal ValueError with no mention of the missing emp_id_col
Status:    FIXED (2026-03-31) — emp_id_col: Optional[str] = None added to both
           signatures; forwarded to _resolve_survey()

Description:
  Both functions call _resolve_survey(survey) without forwarding an emp_id_col:

    get_response_dist():  data, all_questions, _ = _resolve_survey(survey)
    get_correlations():   data, questions, _    = _resolve_survey(survey)

  Their signatures have no emp_id_col parameter.  When survey is a plain
  DataFrame, _resolve_survey calls extract_questions(data, None), which raises:

    ValueError: emp_id_col must be specified. When loading with
                read_glint_survey(), pass emp_id_col to store it automatically.

  Both docstrings state the survey parameter accepts "A GlintSurvey or plain
  DataFrame", making the API contract clear.  The actual behaviour breaks that
  contract.  Compare summarize_survey(), which correctly includes emp_id_col in
  its signature and forwards it.

Affected lines:
  vivaglint/analyze.py:255   data, all_questions, _ = _resolve_survey(survey)
  vivaglint/analyze.py:480   data, questions, _     = _resolve_survey(survey)

  Also missing from function signatures:
  vivaglint/analyze.py:219   def get_response_dist(survey, questions="all", ...)
  vivaglint/analyze.py:411   def get_correlations(survey, method=..., ...)

Reproduction:
  import pandas as pd
  from vivaglint.analyze import get_response_dist, get_correlations

  df = pd.read_csv("tests/fixtures/test_survey_1000.csv")
  get_response_dist(df)          # ValueError: emp_id_col must be specified
  get_correlations(df)           # ValueError: emp_id_col must be specified

Test gap:
  TestGetResponseDist and TestGetCorrelations have no test_from_plain_dataframe
  test (unlike TestSummarizeSurvey which has one).  All tests use the
  session-scoped GlintSurvey fixture, masking the broken plain-DataFrame path.

Fix:
  Add emp_id_col: Optional[str] = None to both function signatures.
  Forward it to _resolve_survey():
    data, all_questions, _ = _resolve_survey(survey, emp_id_col)
  Also add test_from_plain_dataframe tests to each class.


================================================================================
BUG 20 — LOW: get_response_dist() pct values are full-precision floats,
              diverging from R parity and from summarize_survey convention
================================================================================

Function:  vivaglint/analyze.py :: get_response_dist(), line 273
Severity:  Low — minor R parity gap; unlikely to cause downstream failures
Status:    Open

Description:
  summarize_survey() rounds all pct_ columns to 1 decimal place:

    pct_favorable = round((n_favorable / n_responses) * 100, 1)

  get_response_dist() stores raw full-precision floats:

    value_pcts[v] = float(cnt / total * 100)   # no rounding

  The R source uses round(..., 1) for both functions.  The difference is not
  caught by the existing test because test_pct_cols_sum_to_100 rounds the sum
  before asserting:

    assert (pct_sums.round(1) == 100.0).all()

  This masks individual values that are unrounded.

Affected line:
  vivaglint/analyze.py:273
    value_pcts[v] = float(cnt / total * 100)

Fix:
  Round to 1dp to match summarize_survey and R behaviour:
    value_pcts[v] = round(float(cnt / total * 100), 1)

  Also add a test that checks individual pct values are rounded to 1dp
  (similar to TestSummarizeSurvey.test_pct_rounded_to_1dp).


================================================================================
BUG 21 — LOW: extract_survey_factors() docstring references stale class name
================================================================================

Function:  vivaglint/analyze.py :: extract_survey_factors(), lines 619-621
Severity:  Low — documentation bug; no functional impact
Status:    FIXED (2026-03-31) — docstring updated; fa_object now references _VivaGlintFA

Description:
  After the Bug 1 fix replaced factor_analyzer with _VivaGlintFA, the docstring
  for the "fa_object" return key still says:

    * ``"fa_object"`` — the raw
      :class:`factor_analyzer.FactorAnalyzer` instance.

  The actual object is now a vivaglint._factor_analysis._VivaGlintFA instance.
  Any caller who checks type(result["fa_object"]) or uses FactorAnalyzer-
  specific attributes will be confused.

Affected lines:
  vivaglint/analyze.py:619-621

Fix:
  Update the docstring:
    * ``"fa_object"`` — the fitted
      :class:`~vivaglint._factor_analysis._VivaGlintFA` instance.

  Also check if any existing tests assert the type of fa_object — none do,
  but a test_fa_object_type test would guard against future regressions.


================================================================================
BUG 22 — LOW: test_change_formula in TestCompareCycles is also vulnerable
              to the Bug 6 lexicographic sort (will fail with 10+ cycles)
================================================================================

Test:      tests/test_analyze.py :: TestCompareCycles.test_change_formula
Severity:  Low — test correctness depends on 2-cycle scenario only;
           would silently pass wrong change values with 10+ cycles
Status:    Open / Test robustness gap

Description:
  test_change_formula asserts that cycle2's change_from_previous equals
  mean(cycle2) - mean(cycle1) per question.  It sorts the filtered rows by
  "cycle" inside the test to get cycle1 then cycle2:

    q_data = result[result["question"] == q].sort_values("cycle")
    c1_mean = q_data.iloc[0]["mean"]
    c2_mean = q_data.iloc[1]["mean"]

  With 2 cycles this lexicographic sort happens to be correct ("Cycle 1" < "Cycle 2").
  If the test were extended to verify correctness with 10+ cycles (which is the
  right way to test Bug 6's fix), this internal sort would also produce wrong
  order — "Cycle 10" before "Cycle 2" — causing the change assertion to fail for
  the wrong reason, masking the real issue.

  Additionally, the test has no tolerance guard: it uses `< 1e-10` for floating-
  point equality, which could fail if cycle summaries accumulate numerical noise.

Affected line:
  tests/test_analyze.py:235
    q_data = result[result["question"] == q].sort_values("cycle")

Fix (test hardening):
  Use explicit cycle name filtering instead of positional indexing:
    c1_row = q_data[q_data["cycle"] == "Cycle 1"].iloc[0]
    c2_row = q_data[q_data["cycle"] == "Cycle 2"].iloc[0]
    assert abs(c2_row["change_from_previous"] - (c2_row["mean"] - c1_row["mean"])) < 1e-10


================================================================================
BUG 23 — MEDIUM: _VivaGlintFA._oblimin() produces degenerate factor solutions
                  for all n_factors values (oblimin/quartimin only)
================================================================================

Function:  vivaglint/_factor_analysis.py :: _VivaGlintFA._oblimin()
Severity:  Medium — only affects users who explicitly pass rotation="oblimin" or
           "quartimin" and ignore the UserWarning; default (varimax) is unaffected
Status:    PARTIALLY MITIGATED (2026-03-31) — default rotation changed to "varimax";
           oblimin/quartimin now emit UserWarning when explicitly requested.
           Underlying algorithm still broken — proper fix (Option B) pending.

Description:
  The gradient-descent oblimin implementation has two failure modes:

  MODE 1 — Small n_factors (2, 3, 4):
    The transformation matrix T collapses to rank-1.  All columns of T converge
    to the same direction (or its negative), so every pattern loading column
    becomes ±identical:

      n_factors=2  →  MR1 loadings == -MR2 loadings (r = -1.000)
      n_factors=3  →  all 3 columns collinear
      n_factors=4  →  all 4 columns collinear

    Root cause: after each gradient step the columns of T are individually
    renormalised to unit length.  Nothing prevents two columns from converging
    to the same unit vector.  When T is rank-1, pinv(T) is unreliable and
    the pattern matrix L = A @ pinv(T).T contains only tiny values (all
    below 0.3), so factor_summary is empty.

  MODE 2 — Larger n_factors (6, default Kaiser output):
    T is technically full-rank but has a condition number of ~235,000
    (a well-conditioned matrix has cond ≈ 1–10).  The pattern loadings are
    capped by the rescaling step (max abs loading per factor = exactly 1.0
    for all factors), indicating the uncapped values exceeded 1.0.  The
    resulting factor structure is numerically unstable and should not be
    interpreted.

  Because oblimin is the DEFAULT rotation for extract_survey_factors(), every
  default call returns either an empty factor_summary or a numerically unstable
  one.  The existing tests do not detect this because they only check:
    - loading ∈ [−1, 1]  (passes — values are capped to this range)
    - communality ∈ [0, 1]  (passes — stored pre-rotation)
  No test checks that factors are non-collinear or that T is well-conditioned.

Reproduction:
  from vivaglint import read_glint_survey, extract_survey_factors
  survey = read_glint_survey("tests/fixtures/test_survey_1000.csv", emp_id_col="EMP ID")

  # Default call (oblimin, Kaiser n_factors) — condition number 235,000:
  r = extract_survey_factors(survey)
  # Appears to work (25 rows) but loadings are numerically garbage

  # Explicit n_factors=2 with oblimin — empty result:
  r2 = extract_survey_factors(survey, n_factors=2, rotation="oblimin")
  assert len(r2["factor_summary"]) == 0   # True — all loadings below 0.3

  # Confirmed working rotations (non-oblimin):
  r3 = extract_survey_factors(survey, n_factors=2, rotation="varimax")
  # Produces sensible loadings (max ~0.91)

Affected lines:
  vivaglint/_factor_analysis.py:282-339  (_oblimin method)

  The core flaw is the column-normalisation constraint at line 322-324:
    col_norms = np.linalg.norm(T_new, axis=0)
    col_norms = np.where(col_norms < 1e-12, 1.0, col_norms)
    T_new = T_new / col_norms
  This allows all columns to converge to the same unit vector.

Fix:
  The gradient-descent oblimin implementation needs to be replaced with a
  numerically stable algorithm.  Two options:

  Option A — Use scipy's built-in orthogonal/oblique rotation:
    scipy.linalg does not include oblimin, but the orthomax family (varimax,
    quartimax) can be used for orthogonal rotations.

  Option B — Implement the standard Bernaard-Jennrich oblimin algorithm:
    Replace the gradient-descent loop with the closed-form LS-update iteration
    used in R's GPArotation package (L-BFGS or the alternating projected
    gradient method with proper rank-preservation constraints).

  Interim workaround:
    Change the default rotation from "oblimin" to "varimax" in
    extract_survey_factors() until oblimin is fixed.  Varimax is confirmed
    to produce correct results.


================================================================================
BUG 24 — HIGH: analyze_by_attributes() shallow-copy leaves metadata shared;
                original survey.metadata["attribute_cols"] still mutated
================================================================================

Function:  vivaglint/analyze.py :: analyze_by_attributes(), line 883
Severity:  High — silent data corruption: the original GlintSurvey's metadata
           is modified even when the caller passes attribute_file=; callers
           who check survey.metadata["attribute_cols"] after the call will see
           unexpected attribute columns
Status:    FIXED (2026-03-31) — copy.deepcopy(survey) applied instead of
           copy.copy(survey); deepcopy creates an independent metadata dict

Description:
  The Bug 2 fix replaced the direct join_attributes call with:

    survey = join_attributes(copy.copy(survey), attribute_file, ...)

  copy.copy() creates a shallow copy of the GlintSurvey object.  The metadata
  attribute of the copy is the SAME dict object as in the original, so when
  join_attributes does:

    survey.metadata["attribute_cols"] = list(...)

  it writes into the shared dict and the original survey's attribute_cols is
  mutated.  survey.data was correctly protected (join_attributes does
  data = survey.data.copy() internally), but metadata was not.

Affected line:
  vivaglint/analyze.py:883
    survey = join_attributes(copy.copy(survey), ...)   # was copy.copy

Fix applied:
  Change copy.copy to copy.deepcopy so the metadata dict is fully independent.

Regression test:
  test_regression.py::TestBug2AnalyzeByAttributesDoesNotMutateSurvey::
    test_original_attribute_cols_metadata_unchanged


================================================================================
BUG 25 — MEDIUM: get_all_reports() can return the queried manager in the result
                  when the org chart contains a cycle
================================================================================

Function:  vivaglint/hierarchy.py :: _get_all_reports_impl(), lines 83-104
Severity:  Medium — incorrect result (manager appears as their own report);
           no crash, no warning; silent wrong output for cyclic org charts
Status:    Open

Description:
  The cycle guard adds each visited manager_id to a _visited set and returns []
  if the same id is encountered again during recursion.  This prevents infinite
  loops.  However, the original manager_id is added to _visited ONLY for the
  purpose of blocking re-entry, not for filtering the final result.

  When the org chart has a cycle (e.g. A manages B, B manages C, C manages A),
  the traversal starting at A proceeds:

    _visited = {A}
    direct_reports(A) = [B]
    all_reports = [B]
    recurse(B, _visited={A,B}):
      direct_reports(B) = [C]
      all_reports = [C]
      recurse(C, _visited={A,B,C}):
        direct_reports(C) = [A]     ← A is a direct report of C
        all_reports = [A]           ← A is ADDED to the result list
        recurse(A, _visited={...}): ← A is in _visited → return []
      return [C, A]
    return [B, C, A]               ← A appears in its own report list

  The fixture data (1000 employees) contains at least one such cycle, confirmed
  by test_edge_cases.py::TestGetAllReportsEdgeCases::test_result_excludes_manager_itself.

Affected line:
  vivaglint/hierarchy.py:83-104

Reproduction:
  import pandas as pd
  from vivaglint.hierarchy import get_all_reports
  df = pd.DataFrame({"EMP ID": ["A","B","C"], "Manager ID": ["B","C","A"]})
  result = get_all_reports("A", df, "EMP ID", "Manager ID")
  # result = ["B", "C", "A"] — "A" should not appear

Fix:
  After the recursive collection, filter the result to exclude the original
  manager_id:

    return [emp for emp in list(dict.fromkeys(all_reports))
            if emp != manager_id]

  This is safe because an employee cannot genuinely report to themselves.


================================================================================
BUG 26 — MEDIUM: split_survey_data() and extract_questions() include attribute
                  columns as question stems on attribute-enriched surveys
================================================================================

Function:  vivaglint/reshape.py :: split_survey_data(), line 187
           vivaglint/import_.py :: extract_questions(), lines 217-219
Severity:  Medium — silently incorrect output: attribute columns (e.g.
           "Department", "Gender") appear in the quantitative split as if they
           were survey response columns; callers of extract_questions() on an
           enriched GlintSurvey get attribute cols as question stems
Status:    Open

Description:
  After join_attributes() is called, survey.data contains both the original
  10 survey columns AND the joined attribute columns (Department, Gender, etc.).

  split_survey_data() resolves questions via:
    questions = extract_questions(data, emp_id_col)

  where data is the raw (enriched) DataFrame.  extract_questions() filters out
  get_standard_columns(emp_id_col), which does NOT include attribute columns.
  Therefore "Department" passes the filter and is returned as a question stem.

  Consequence in split_survey_data():
    response_cols includes "Department" (it exists in data)
    quant_cols includes "Department"
    quantitative output has a "Department" column

  Same root cause in extract_questions() when called with a GlintSurvey that
  has had attributes joined:
    df = data.data  (enriched)
    question_cols includes attribute cols
    rows include stems for "Department", "Gender", etc.

  Note: the analysis functions (summarize_survey, get_correlations, etc.) are
  NOT affected because they use _resolve_survey(), which reads questions from
  survey.metadata["questions"] — populated before attributes were joined.

  Only direct calls to extract_questions() and split_survey_data() are affected.

Affected lines:
  vivaglint/import_.py:217-219   (extract_questions — plain df path)
  vivaglint/reshape.py:187       (split_survey_data — calls extract_questions(data, ...))

Reproduction:
  survey = read_glint_survey("tests/fixtures/test_survey_1000.csv", emp_id_col="EMP ID")
  join_attributes(survey, "tests/fixtures/test_attributes_1000.csv")
  from vivaglint.reshape import split_survey_data
  result = split_survey_data(survey)
  print("Department" in result["quantitative"].columns)  # True — wrong

Regression tests:
  test_edge_cases.py::TestSplitSurveyDataWithAttributes (xfail)
  test_edge_cases.py::TestExtractQuestionsAttributeEnriched (xfail)

Fix:
  Option A — Pass the GlintSurvey (not just data) to extract_questions in
    split_survey_data, and have extract_questions return metadata["questions"]
    when the input is a GlintSurvey (it already does — but split_survey_data
    currently passes the raw DataFrame, bypassing this path).

    In split_survey_data, change:
      questions = extract_questions(data, emp_id_col)
    to:
      questions = extract_questions(survey, emp_id_col) if isinstance(survey, GlintSurvey) ...

  Option B — In extract_questions(), when the input is a GlintSurvey, exclude
    columns listed in survey.metadata["attribute_cols"] from question_cols.


================================================================================
WARNINGS (non-breaking, observed during test run)
================================================================================

W1. pandas FutureWarning — infer_datetime_format (see Bug 3 above)
    Fires 8 times during test_analyze.py::TestAnalyzeAttrition tests.

W2. UserWarning from join_attributes when re-joining attributes to a survey that
    already has attribute columns:
      "The following columns already exist in the survey data and will be
       overwritten: 'Department', 'Gender', ..."
    This fires during test_import.py::TestJoinAttributes when the session-scoped
    survey fixture already has attributes joined. Working as intended — the
    warning is correct and informative. No action needed.


================================================================================
ENVIRONMENT
================================================================================

Python:            3.13.12
pytest:            9.0.2
pandas:            2.3.3
numpy:             2.4.0
scipy:             1.17.1
factor_analyzer:   0.5.1  (incompatible with scikit-learn 1.6+)
scikit-learn:      1.8.0
rapidfuzz:         (installed, used by search_comments)
matplotlib:        (installed)
seaborn:           (installed)
Platform:          Windows 11 Enterprise (win32)
