Skip to content

feat: fix loaders some more #98

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.

Sign up for GitHub

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

Merged
merged 4 commits into from
Dec 19, 2024
Merged

feat: fix loaders some more #98

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
f300b6e
fix: don't need to concatenate matching file to the extracted feature…
strixy16 Dec 18, 2024
2841929
test: add basic test of load feature files function
strixy16 Dec 18, 2024
4c58b7f
build: latest readii version updated
strixy16 Dec 18, 2024
1d84838
test: got test function working with tmp_path_factory
strixy16 Dec 19, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions pixi.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

9 changes: 3 additions & 6 deletions src/readii/io/loaders/features.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ def loadFeatureFilesFromImageTypes(extracted_feature_dir:Union[Path|str], # noqa
logger.error(f"Extracted feature directory {extracted_feature_dir} does not exist.")
raise FileNotFoundError()

# Get list of all the csv files in the directory with their full paths
feature_file_list = sorted(extracted_feature_dir.glob("*.csv"))

# Loop through all the files in the directory
Expand All @@ -69,17 +70,13 @@ def loadFeatureFilesFromImageTypes(extracted_feature_dir:Union[Path|str], # noqa
msg = f"Multiple {image_type} feature csv files found in {extracted_feature_dir}. First one will be used."
logger.warning(msg)

image_type_feature_file = matching_files[0]
feature_file_path = matching_files[0]
# Remove the image type file from the list of feature files
feature_file_list.remove(image_type_feature_file)
feature_file_list.remove(feature_file_path)
Comment on lines +73 to +75
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Avoid modifying the feature_file_list during iteration

Modifying the list while processing it could lead to race conditions or missed files. Consider using a set or maintaining a separate list of processed files.

-            feature_file_path = matching_files[0]  
-            # Remove the image type file from the list of feature files  
-            feature_file_list.remove(feature_file_path)
+            feature_file_path = matching_files[0]
+            # Keep track of processed files in a separate set
+            processed_files.add(feature_file_path)

Also, consider adding this at the beginning of the function:

processed_files = set()


except Exception as e:
logger.warning(f"Error loading {image_type} feature csv files from {extracted_feature_dir}: {e}")
raise e
Comment on lines 77 to 79
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Improve error handling consistency

The current error handling logs a warning and then raises the exception. This is inconsistent with other error handling in the function. Consider either:

  1. Logging an error and raising a specific exception, or
  2. Logging a warning and continuing to the next iteration
-        except Exception as e:
-            logger.warning(f"Error loading {image_type} feature csv files from {extracted_feature_dir}: {e}")
-            raise e
+        except Exception as e:
+            logger.error(f"Failed to process {image_type} feature files: {e}")
+            raise ValueError(f"Error processing {image_type} feature files: {e}") from e
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
except Exception as e:
logger.warning(f"Error loading {image_type} feature csv files from {extracted_feature_dir}: {e}")
raise e
except Exception as e:
logger.error(f"Failed to process {image_type} feature files: {e}")
raise ValueError(f"Error processing {image_type} feature files: {e}") from e



# Get the full path to the feature file
feature_file_path = extracted_feature_dir / image_type_feature_file

# Load the feature data into a pandas dataframe
raw_feature_data = loadFileToDataFrame(feature_file_path)
Expand Down
15 changes: 15 additions & 0 deletions tests/io/loaders/test_features.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
import pytest

from pathlib import Path
from readii.io.loaders.features import loadFeatureFilesFromImageTypes


@pytest.fixture
def extracted_feature_4D_lung():
return Path("tests/4D-lung/results/features/")

def test_loadFeatureFilesFromImageTypes(extracted_feature_4D_lung):
feature_sets = loadFeatureFilesFromImageTypes(extracted_feature_4D_lung, image_types=["original"], drop_labels=False)
assert isinstance(feature_sets, dict), "Return should be a dictionary."
assert len(feature_sets) == 1, "Should only have one feature file per image type."
assert "original" in feature_sets, "Not finding the specified image type feature file (original) or keys are not correctly named."
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Expand test coverage with additional test cases

The current test only covers the happy path with a single image type. Consider adding:

  1. Test with multiple image types
  2. Test with non-existent image types
  3. Test with drop_labels=True
  4. Test error conditions (invalid directory, invalid files)

Example additional test:

def test_loadFeatureFilesFromImageTypes_multiple_types(extracted_feature_4D_lung):
    feature_sets = loadFeatureFilesFromImageTypes(
        extracted_feature_4D_lung, 
        image_types=["original", "wavelet"],
        drop_labels=True
    )
    assert len(feature_sets) == 2
    assert all(key in feature_sets for key in ["original", "wavelet"])

def test_loadFeatureFilesFromImageTypes_invalid_type(extracted_feature_4D_lung):
    feature_sets = loadFeatureFilesFromImageTypes(
        extracted_feature_4D_lung,
        image_types=["nonexistent"],
        drop_labels=False
    )
    with pytest.raises(ValueError):
        assert not feature_sets

Loading