Skip to content

Conversation

Copy link
Contributor

Copilot AI commented Oct 28, 2025

Plan to merge RBMDataset and RBMDatasetElpaso classes

  • Analyze existing RBMDataSet and RBMDataSetElPaso classes
  • Review tests and understand usage patterns
  • Run existing tests to confirm current state (72 tests passing)
  • Design merged class that supports both file loading and dict loading
  • Implement merged RBMDataSet class with optional parameters
  • Update update_from_dict method in RBMDataSet
  • Add variable mapping functionality to RBMDataSet
  • Update init.py to remove RBMDataSetElPaso export
  • Update imports in bin_and_interpolate_to_model_grid.py
  • Update all tests to use merged RBMDataSet class
  • Remove RBMDataSetElPaso.py file
  • Run all tests to ensure functionality is preserved (135 tests passing)
  • Run linters and fix any issues
  • Code review completed - no issues found
  • Security check completed - no vulnerabilities
  • Tests updated to match refactored implementation (136 tests passing)
  • Merge test files into single test_RBMDataset.py

✅ Task Complete: Successfully Merged and Refactored RBMDataSet

Summary of Changes

Original Merge: Combined RBMDataSet and RBMDataSetElPaso into a unified class supporting both file-based and dictionary-based loading.

Refactoring by @sahiljhawar:

  • Replaced variable mapping dictionary with direct VariableLiteral usage
  • Simplified update_from_dict() to use direct key matching
  • Added DummyEnum for generic type handling
  • Improved error handling with VariableNotFoundError
  • Added find_similar_variable() helper for better error messages

Test Updates:

  • Removed MockVariable wrapper class
  • Updated all tests to use VariableLiteral keys directly
  • Simplified test data to pass arrays instead of wrapper objects
  • Added tests for invalid key error handling
  • Consolidated test files: Merged test_RBMDatasetElPaso.py into test_RBMDataset.py - single test file now contains all 73 tests for the unified RBMDataSet class

Test Organization

test_RBMDataset.py now contains:

  1. File-based loading tests - Tests for loading data from files with start_time, end_time, and folder_path
  2. Dictionary-based loading tests - Tests for loading data via update_from_dict() (prefixed with test_dict_mode_)

All 136 tests passing across all RBMDataSet test files ✅

Usage Examples

File Loading Mode:

dataset = RBMDataSet(
    satellite=SatelliteEnum.RBSPA,
    instrument=InstrumentEnum.MAGEIS,
    mfm=MfmEnum.T89,
    start_time=datetime(2023, 1, 1),
    end_time=datetime(2023, 1, 31),
    folder_path=Path("/data/path")
)

Dictionary Loading Mode:

dataset = RBMDataSet(
    satellite=SatelliteEnum.GOESSecondary,
    instrument=InstrumentEnum.MAGED,
    mfm=MfmEnum.T89
)
# Pass data with VariableLiteral keys directly
data_dict = {
    "Flux": np.array([[1.0, 2.0, 3.0]]),
    "energy_channels": np.array([100.0, 200.0, 300.0]),
    "time": np.array([738000.0])
}
dataset.update_from_dict(data_dict)

Migration Notes

For code using the merged class with dictionary loading:

  1. Use VariableLiteral keys directly (e.g., "Flux", "energy_channels", "time")
  2. Pass data arrays directly as values (no wrapper objects needed)
  3. Invalid keys will raise VariableNotFoundError with helpful suggestions

All tests passing: 136 tests across all RBMDataSet test files ✅

Original prompt

RBMDataset and RBMDatasetElpaso needs to be merged, such that there will exist only one class in which either the user can provide the file location for data to load or the data can be loaded using dict method which is currenlty in RBMDatasetElpaso


💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.

Copilot AI and others added 3 commits October 28, 2025 17:54
…taSet class

Co-authored-by: sahiljhawar <55475299+sahiljhawar@users.noreply.github.com>
Co-authored-by: sahiljhawar <55475299+sahiljhawar@users.noreply.github.com>
Co-authored-by: sahiljhawar <55475299+sahiljhawar@users.noreply.github.com>
Copilot AI changed the title [WIP] Merge RBMDataset and RBMDatasetElpaso classes Merge RBMDataSet and RBMDataSetElPaso into unified class Oct 28, 2025
Copilot AI requested a review from sahiljhawar October 28, 2025 18:04
- Replace variable mapping dictionary with direct VariableLiteral usage
- Remove ElPaso-specific type annotations and use generic DummyLike types
- Improve error handling for computed properties (P, InvV) with proper validation
- Simplify update_from_dict() method to use direct key matching instead of mapping
- Add find_similar_variable() helper method for better error suggestions
- Remove unused imports and replace ElPasoMFMEnum with DummyEnum
@sahiljhawar
Copy link
Collaborator

@copilot I have made changes to the RBMDataset code, change the tests accordingly.

- Remove MockVariable wrapper class as it's no longer needed
- Update tests to use direct VariableLiteral keys instead of mapped names
- Simplify update_from_dict tests to pass data arrays directly
- Add tests for invalid key error handling
- Remove obsolete variable mapping tests
- All 136 tests passing

Co-authored-by: sahiljhawar <55475299+sahiljhawar@users.noreply.github.com>
Copy link
Contributor Author

Copilot AI commented Oct 29, 2025

@copilot I have made changes to the RBMDataset code, change the tests accordingly.

Updated all tests to match your refactored implementation. The tests now use VariableLiteral keys directly instead of the old mapping approach with MockVariable wrapper objects. All 136 tests are passing. (commit c672e0a)

@sahiljhawar
Copy link
Collaborator

@copilot RBMDataSetElPaso does not exist anymore. There should be only one test file for RBMDataSet which tests all the changed functionality

Since RBMDataSetElPaso no longer exists as a separate class, consolidate all tests into a single test file. The merged file now contains:
- File-based loading tests (original test_RBMDataset.py)
- Dictionary-based loading tests (from test_RBMDatasetElPaso.py)

All 73 tests for RBMDataSet now in one file with clear naming:
- Original tests remain unchanged
- Dict mode tests prefixed with "test_dict_mode_" for clarity
- Total: 136 tests passing across all RBMDataSet test files

Co-authored-by: sahiljhawar <55475299+sahiljhawar@users.noreply.github.com>
Copy link
Contributor Author

Copilot AI commented Oct 29, 2025

@copilot RBMDataSetElPaso does not exist anymore. There should be only one test file for RBMDataSet which tests all the changed functionality

Merged test_RBMDatasetElPaso.py into test_RBMDataset.py. All 73 tests for the unified RBMDataSet class are now in a single file, with dictionary-based loading tests prefixed with test_dict_mode_ for clarity. All 136 tests passing. (commit 4044cfd)

- Remove obsolete test_dict_mode_variable_mapping_exposed function
- Add get_loaded_variables() method to return list of currently loaded variable names
- Refactor __eq__ method to handle both file loading and dict variable
@sahiljhawar sahiljhawar marked this pull request as ready for review October 29, 2025 15:38
@sahiljhawar sahiljhawar merged commit c98902d into main Nov 4, 2025
7 of 8 checks passed
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