Skip to content

Conversation

@rburghol
Copy link
Contributor

@rburghol rburghol commented Jan 2, 2026

This is a prototype to allow an update of settings ONLY from a UCI into an hdf5 that has already been created with hsp2 import_uci ....

  • Copies parameters over, but omits timeseries data like WDM, mutsin ...
  • This results in a huge time savings when testing, and I can only imagine that if I were developing a model that was UCI based, I would be very grateful for this feaure.
  • I consider this ready to merge as it DOES work initially, but caveats follow.
  • This works well the first time, but subsequent attempts fail when trying to run due to duplicate column names that occur due to renaming in the function readUCI()
  • See this issue for details on the problems with this function after the 2nd update attempt: Bug: readUCI() renames columns before setting defaults, which creates duplicate columns #201

@rburghol
Copy link
Contributor Author

rburghol commented Jan 6, 2026

@timcera - I am putting this here as the fork on your github account does not allow Issues to be posted (happy to put that over there if you prefer).

In my opinion, the code in your IOManager class in the __init__() method beginning at line 104 should be split out into the child sub-classes for hdf5, csv, and uci. In other words, those should be actual class definitions (not just supplying arguments to the IOManager class) each have their own __init__() method that handles the validation that you are doing, then calls the parent/super-class method after handling validation, so the base class for IOManager should be agnostic of what the sub-classes are doing. For example, the CSVIOManager would have a method like so :


class CSVIOManager(IOManager):
    def __init__(self):
        super(CSVIOManager, self).__init__()

   def read_parameters(self):
       return read_csv_parameters()
    
    def write_ts(
        self,
        data_frame: pd.DataFrame,
        save_columns: List[str],
        category: Category,
        operation: Union[str, None] = None,
        segment: Union[str, None] = None,
        activity: Union[str, None] = None,
        outstep: int = 2,
        *args,
        **kwargs,
    ) -> None:
    
...

I realize that the IOManager class as developed allows this replacing methods via function passing, but to me it seems less straightforward, and hard to read -- I personally prefer a straight-forward parent class, child class, child class supplies what needs to be different very explicitly, in methods that are defined in the same file.

But even if the preferred way of creatin these child classes is to do them with this type of declaration, IMO if you are going to do validation, you shouldn't have to make the parent class aware of the potential of the child class in order to support the child class via a series of if-then's in the parent class.

I may be wrong about this, and would be interested in @austinorr opinion or insight.

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