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

Unify datasets part 1 #2288

Merged
merged 9 commits into from
Jul 24, 2024
Merged

Unify datasets part 1 #2288

merged 9 commits into from
Jul 24, 2024

Conversation

samtygier-stfc
Copy link
Collaborator

@samtygier-stfc samtygier-stfc commented Jul 22, 2024

Issue

Work on #2199

Description

This is the first step in unifying MixedDataset and StrictDataset. This first step moves all the implementation of MixedDataset into BaseDataset.

This required some changes that result in a small changes to the rest of the codebase
Forcing constructor arguments to named, because the current classes use different argument ordering
Forcing access to the recons through a property.

Testing

I have moved the MixedDataset and StrictDataset tests into core.
I have added a DatasetTest, currently for BaseDataset. This will duplicate tests from the other test classes, and eventually replace them.
I have left the MixedDataset and StrictDataset alone (apart from small API changes), to ensure that the classes behaviour is not changed.

Acceptance Criteria

Test loading a dataset and an image stack

Documentation

Not needed yet

@samtygier-stfc samtygier-stfc force-pushed the 2199-unify-datasets-1 branch from 453431e to cf7bf35 Compare July 22, 2024 09:52
@coveralls
Copy link

Coverage Status

coverage: 73.09% (-0.03%) from 73.115%
when pulling cf7bf35 on 2199-unify-datasets-1
into 8cf298f on main.

@samtygier-stfc samtygier-stfc marked this pull request as ready for review July 22, 2024 10:12
@JackEAllen JackEAllen self-requested a review July 23, 2024 08:35
Copy link
Collaborator

@JackEAllen JackEAllen left a comment

Choose a reason for hiding this comment

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

Having now tested a few times and gone over the code a few times, I'm happy to approve this. If you feel that a second courtesy review is needed, I would also agree with this given the code we are touching, otherwise can just merge in :)

@samtygier-stfc
Copy link
Collaborator Author

I think its all going to get a bunch more testing over the next few PRs, so I'd suggest merging now

@samtygier-stfc samtygier-stfc added this pull request to the merge queue Jul 24, 2024
Merged via the queue into main with commit 45416e7 Jul 24, 2024
8 checks passed
@samtygier-stfc samtygier-stfc deleted the 2199-unify-datasets-1 branch July 24, 2024 15:35
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.

3 participants