not closing files prevents re-opening them #901
Conversation
📝 WalkthroughWalkthroughThe changes involve modifications to the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Run
User->>Run: call times(qty)
Run->>Run: call all_times()
Run->>HDF5: process time keys
HDF5-->>Run: return time data
Run->>Run: store time data in dictionary
Run-->>User: return time array for qty
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
pyphare/pyphare/pharesee/run/run.py (2)
Line range hint
268-287: Improvements in resource management and code structure.The changes in the
all_timesmethod are beneficial:
- The loop indentation has been corrected, ensuring all files are processed.
- The HDF5 file is now properly closed after processing, preventing resource leaks.
These improvements enhance the method's reliability and resource management.
However, consider the following suggestions for further improvement:
- Add error handling for file operations and key lookups to make the code more robust.
- Consider moving the
quantities_per_filedictionary to a class attribute for better encapsulation.Example of error handling and using a class attribute:
class Run: quantities_per_file = { "EM_B": "B", "EM_E": "E", "ions_bulkVelocity": "Vi", "ions_density": "Ni", "particle_count": "nppc", } def all_times(self): from glob import glob import os import h5py files = self.available_diags ts = {} for file in files: basename = os.path.basename(file).split(".")[0] try: with h5py.File(file, 'r') as ff: time_keys = ff["t"].keys() time = np.zeros(len(time_keys)) for it, t in enumerate(time_keys): time[it] = float(t) ts[self.quantities_per_file[basename]] = time except (IOError, KeyError) as e: logger.error(f"Error processing file {file}: {str(e)}") return ts
289-290: New method for accessing quantity-specific time data.The addition of the
timesmethod is a good improvement, providing a convenient way to access time data for a specific quantity.To enhance its robustness and usability, consider the following suggestions:
- Add error handling to gracefully manage cases where the specified quantity doesn't exist.
- Include a docstring to explain the method's purpose, parameters, and return value.
Here's an example of how you might implement these suggestions:
def times(self, qty): """ Retrieve the time array for a specified quantity. Args: qty (str): The quantity for which to retrieve the time array. Returns: numpy.ndarray: The time array for the specified quantity. Raises: KeyError: If the specified quantity is not found in the available data. """ all_times_data = self.all_times() if qty not in all_times_data: raise KeyError(f"Quantity '{qty}' not found in available data.") return all_times_data[qty]
Summary by CodeRabbit
New Features
Bug Fixes