Skip to content

refactor: move to popsummary format to store probs#850

Merged
IamMuhammadZeeshan merged 3 commits into
mainfrom
move-to-popsummary-format
Jun 16, 2026
Merged

refactor: move to popsummary format to store probs#850
IamMuhammadZeeshan merged 3 commits into
mainfrom
move-to-popsummary-format

Conversation

@Qazalbash

Copy link
Copy Markdown
Member

No description provided.

@Qazalbash Qazalbash self-assigned this Jun 15, 2026
@Qazalbash Qazalbash added the internal An internal refactor or improvement label Jun 15, 2026

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

This pull request integrates the popsummary library to manage population results and grid rates, replacing custom domain reading/writing logic and raw HDF5 operations in marginals.py. It also adds support for numpy types when reading HDF5 attributes in utils.py. The review feedback recommends converting filepath to a string for compatibility, explicitly closing PopulationResult objects to prevent file locking, using a more robust list comprehension for constructing hyperparameters, ensuring domains keys are cast to strings, and correcting a type annotation for pos_and_rates.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment thread src/gwkokab/analysis/utils/marginals.py
Comment on lines +291 to +297
)

for i in range(N_components):
comp_i_group = probs_group.create_group(f"component_{i}")
for idx, param in enumerate(parameters):
write_to_hdf5(comp_i_group, param, np.array(batched_results[i][idx]))
write_to_hdf5(
filepath,
dataset_path="/posterior/hyperparameter_samples",
attrs={"constants": constants, "variables_index": variables_index},
)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

high

Explicitly closing the PopulationResult object ensures that all writes are flushed to disk and the file handle is released before write_to_hdf5 attempts to open the same file. This prevents potential file locking issues or write conflicts.

Suggested change
)
for i in range(N_components):
comp_i_group = probs_group.create_group(f"component_{i}")
for idx, param in enumerate(parameters):
write_to_hdf5(comp_i_group, param, np.array(batched_results[i][idx]))
write_to_hdf5(
filepath,
dataset_path="/posterior/hyperparameter_samples",
attrs={"constants": constants, "variables_index": variables_index},
)
)
result.close()
write_to_hdf5(
filepath,
dataset_path="/posterior/hyperparameter_samples",
attrs={"constants": constants, "variables_index": variables_index},
)

Comment thread src/gwkokab/analysis/utils/marginals.py
Comment thread src/gwkokab/analysis/utils/marginals.py
Comment thread src/gwkokab/analysis/utils/marginals.py Outdated
Comment thread src/gwkokab/analysis/utils/marginals.py
IamMuhammadZeeshan and others added 2 commits June 15, 2026 21:36
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
@IamMuhammadZeeshan IamMuhammadZeeshan merged commit cbf043a into main Jun 16, 2026
5 checks passed
@IamMuhammadZeeshan IamMuhammadZeeshan deleted the move-to-popsummary-format branch June 16, 2026 23:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

internal An internal refactor or improvement

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants