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

Refactor sections property #197

Merged
merged 5 commits into from
Aug 14, 2023
Merged

Refactor sections property #197

merged 5 commits into from
Aug 14, 2023

Conversation

BSchilperoort
Copy link
Collaborator

  • sections is now a required argument when calling calibration_*_ended
  • only the calibration methods set the sections attr

Closes #196 :
I also added markers to slow tests. By skipping the slow tests, the test suite runs in 1 minute on my machine. This is a lot faster than with all tests, which took >5 minutes.

I skipped the 6 slowest ones. This skips some of the variance tests, but not all of them, so that should be fine when trying to debug other parts of the code base.

afbeelding

@BSchilperoort
Copy link
Collaborator Author

The notebooks are broken, but rewriting those might be best at the end of all refactoring to avoid doing double work.

Copy link
Collaborator

@bdestombe bdestombe left a comment

Choose a reason for hiding this comment

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

Please have a look at the suggestion for changing the name of set_sections() to set_sections_to_ds(). Great PR that helps us step by step moving closer to the implementation of the accessor!

from dtscalibration.datastore_utils import ufunc_per_section_helper


def set_sections(ds: xr.Dataset, sections: Dict[str, List[slice]]) -> xr.Dataset:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe change the name to set_sections_to_ds() to better describe what it does.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That feels a bit redundant to me. It's a function that takes a dataset and sections, and returns a dataset.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ok! Let's merge

@bdestombe
Copy link
Collaborator

The notebooks are broken, but rewriting those might be best at the end of all refactoring to avoid doing double work.

But readthedocs uses the current main branch, such that it will fail if this PR is merged.

@BSchilperoort
Copy link
Collaborator Author

The notebooks are broken, but rewriting those might be best at the end of all refactoring to avoid doing double work.

But readthedocs uses the current main branch, such that it will fail if this PR is merged.

I'll change that to the release version. Which is better anyway, then the docs align with what people install with pip install.

from dtscalibration.datastore_utils import ufunc_per_section_helper


def set_sections(ds: xr.Dataset, sections: Dict[str, List[slice]]) -> xr.Dataset:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Ok! Let's merge

@BSchilperoort BSchilperoort merged commit 0521be8 into main Aug 14, 2023
11 of 12 checks passed
@BSchilperoort BSchilperoort deleted the refactor-sections-prop branch August 14, 2023 08:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Mark slow tests to speed up development time
2 participants