Skip to content
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.

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

Test/save_dataset #72

Merged
merged 32 commits into from
Nov 25, 2024
Merged

Test/save_dataset #72

merged 32 commits into from
Nov 25, 2024

Conversation

siligam
Copy link
Contributor

@siligam siligam commented Nov 19, 2024

Thank you for contributing! 🎉

Summary of the most important changes

Usage example in the PR

Please put a small usage example in the PR body so that we can see what you want to do.

These sorts of examples should also be in the docstrings of your code!

Checklist

  • I have tested the changes in this PR.
  • I have updated the documentation.
  • If I have made a new step which requires new arguments, these are added to the validate.py file
    so that the user can see documentation for the arguments.

Copilot Summary

If you have Github Co-Pilot, please include an automatically generated summary.

@pgierz
Copy link
Member

pgierz commented Nov 19, 2024

The failing tests look like segfaults, and should be handled when #66 is done. I would recommend merging that branch into this one and seeing if it works then.

Copy link
Member

@pgierz pgierz left a comment

Choose a reason for hiding this comment

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

This looks good, but I have some questions and suggestions that you could have a look at.

src/pymorize/files.py Outdated Show resolved Hide resolved
src/pymorize/files.py Outdated Show resolved Hide resolved
src/pymorize/files.py Outdated Show resolved Hide resolved
src/pymorize/files.py Show resolved Hide resolved
src/pymorize/files.py Outdated Show resolved Hide resolved
src/pymorize/files.py Outdated Show resolved Hide resolved
src/pymorize/files.py Show resolved Hide resolved
@pgierz pgierz requested a review from mandresm November 21, 2024 09:41
@pgierz
Copy link
Member

pgierz commented Nov 21, 2024

I'd also like to have @mandresm read through these changes to see that he understands them as well.

@pgierz pgierz self-requested a review November 25, 2024 10:03
Copy link
Member

@pgierz pgierz left a comment

Choose a reason for hiding this comment

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

Some last cleanup, see suggested changes.

src/pymorize/files.py Outdated Show resolved Hide resolved
src/pymorize/frequency.py Outdated Show resolved Hide resolved
tests/integration/test_fesom_2p6_pimesh_esm_tools.py Outdated Show resolved Hide resolved
@pgierz
Copy link
Member

pgierz commented Nov 25, 2024

Copilot Summary

This pull request includes several important changes to the pymorize package, focusing on improving configuration handling, enhancing dataset processing, and updating testing workflows. The most significant changes are grouped into configuration handling improvements, dataset processing enhancements, and testing workflow updates.

Configuration Handling Improvements:

  • src/pymorize/cmorizer.py: Added copy import and modified _post_init_attach_pymorize_config_rules to use copy.deepcopy for configuration to ensure rules get independent copies of the configuration. [1] [2]
  • src/pymorize/config.py: Updated example code to use yaml.dump for writing configuration files, ensuring proper YAML formatting.

Dataset Processing Enhancements:

  • src/pymorize/cmorizer.py: Modified _post_init_populate_rules_with_tables to use tbl.table_id instead of tbl for adding tables to rules.
  • src/pymorize/cmorizer.py: Updated _parallel_process_dask to use deep copies of rules before processing to avoid unintended modifications.
  • src/pymorize/files.py: Added functions to handle time label precision in file names and updated file path creation to ensure directories are created if they do not exist. [1] [2] [3] [4]

Testing Workflow Updates:

These changes collectively improve the robustness and reliability of the pymorize package, particularly in handling configurations and processing datasets.

@pgierz pgierz marked this pull request as ready for review November 25, 2024 10:35
@pgierz pgierz self-requested a review November 25, 2024 11:37
pgierz
pgierz previously approved these changes Nov 25, 2024
@pgierz pgierz merged commit 4c61f1e into main Nov 25, 2024
4 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