Hacktoberfest 2026: những issue maintainer đã đánh dấu cho tháng Mười, đang mở và phù hợp người mới. Xem issue Hacktoberfest

SoilID: Fix component-group selection being sorted alphabetically instead of by cond_prob

Đang mở
  • Có gì không ổn?
#398 1 bình luận 0 reaction 1 người được giao Xem trên GitHub

@johannesparty đang làm issue này rồi.

Từ ngày 24/9/2026.

Đánh giá

Issue này chưa được đánh giá.

Mô tả

bug

Summary

process_distance_scores sorts component-instance rows by descending cond_prob, but then groups them with sort=True. This reorders the groups alphabetically by compname before selecting the first 12 groups, so the function can discard high-probability component groups and retain lower-probability groups solely because their names sort earlier.

Location

soil_id/utils.py, in process_distance_scores:

mucompdata_pd = mucompdata_pd.sort_values(
    ["cond_prob", comp_name_col], ascending=[False, True]
)

mucompdata_comp_grps = [
    g for _, g in mucompdata_pd.groupby([comp_name_col], sort=True)
]
mucompdata_comp_grps = mucompdata_comp_grps[: min(12, len(mucompdata_comp_grps))]

Expected behavior

The selected 12 component groups should be the groups with the highest cond_prob values, preserving the ranking established before grouping.

Actual behavior

sort=True orders the groups by the component name key. Consequently, mucompdata_comp_grps[:12] selects the first 12 groups alphabetically rather than the 12 highest-ranked groups.

Proposed fix

Use sort=False so pandas yields groups in first-appearance order, which follows the preceding cond_prob descending sort:

mucompdata_comp_grps = [
    g for _, g in mucompdata_pd.groupby([comp_name_col], sort=False)
]

Add or update regression tests to verify that the retained groups are the highest-probability component groups, not the alphabetically first groups.

Related issue in the same block

The loop below reassigns and modifies only a local group variable, so its distance sort and min_dist assignment are not reflected in mucompdata_comp_grps or the final concatenated DataFrame:

for group in mucompdata_comp_grps:
    group = group.sort_values(distance_col).reset_index(drop=True)
    group["min_dist"] = group[distance_col].iloc[0]

The implementation should also preserve those intended modifications, for example by assigning the transformed groups back into the list. This should be covered by tests or handled as part of the fix if it is within scope.

@johannesparty regarding our conversation today, we originally truncated the number of soil series (compname) to 12 in the old app due to display issues. It probably makes sense to remove this constraint because, in the US case, many 1000m buffers can contain more than 12 soils.

Ngôn ngữ chính
Python
Star
2
Fork
3
Merge trung bình
4 ngày 13 giờ
Pull request đã merge (30 ngày)
6

Chuẩn bị môi trường

  • Có Dockerfile hoặc tệp Docker Compose
  • Không có mẫu pull request
  • Không có hướng dẫn đóng góp

Bắt đầu từ đâu

  1. Đọc hết issue, rồi đọc hướng dẫn đóng góp của dự án.
  2. Bình luận trên issue rằng bạn sẽ nhận — tránh hai người làm cùng một việc.
  3. Fork repository và làm thay đổi trên một nhánh.
  4. Mở pull request có tham chiếu số hiệu của issue.

Issue khác của techmatters/soil-id-algorithm

Tất cả issue của techmatters/soil-id-algorithm

Issue tương tự

Thêm issue về Python

Nhận issue mới trong hộp thư của bạn

Bản tóm tắt ngắn những issue GitHub phù hợp với người mới.