Skip to content

Ngio projection task #937

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

Merged
merged 25 commits into from
Apr 16, 2025
Merged

Ngio projection task #937

merged 25 commits into from
Apr 16, 2025

Conversation

lorenzocerrone
Copy link
Collaborator

@lorenzocerrone lorenzocerrone commented Apr 10, 2025

  • refactor task core to use ngio > 0.2
  • update current testing
  • add additional testing tailored to flexibility

Checklist before merging

  • I added an appropriate entry to CHANGELOG.md

@lorenzocerrone lorenzocerrone changed the title Ngio projection task Ngio projection task [DO NOT MERGE] Apr 10, 2025
Copy link

github-actions bot commented Apr 10, 2025

Coverage report

Click to see where and how coverage changed

FileStatementsMissingCoverageCoverage
(new stmts)
Lines missing
  fractal_tasks_core/tasks
  copy_ome_zarr_hcs_plate.py 48-49, 193
  projection.py 46
Project Total  

This report was generated by python-coverage-comment-action

Copy link
Collaborator

@jluethi jluethi 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 very promising, great to see how much ngio simplifies things. I added a few minor comments to the task code, will review the tests later :)

Copy link
Collaborator

@jluethi jluethi left a comment

Choose a reason for hiding this comment

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

I added a few very minor comments. Besides that, all looks great and I'd say this PR is ready to be merged into dev-1.5!

@lorenzocerrone lorenzocerrone requested a review from tcompa April 11, 2025 15:39
@lorenzocerrone
Copy link
Collaborator Author

@tcompa I have started refactoring the task tests (see the tests/task_v2). It would be great if you could go over it and look if it all looks alright for you.

@lorenzocerrone lorenzocerrone marked this pull request as ready for review April 11, 2025 15:41
@lorenzocerrone lorenzocerrone changed the title Ngio projection task [DO NOT MERGE] Ngio projection task Apr 11, 2025
@tcompa
Copy link
Collaborator

tcompa commented Apr 14, 2025

I have started refactoring the task tests (see the tests/task_v2). It would be great if you could go over it and look if it all looks alright for you.

Quick generic question: will you include both old (non-tests/tasks_v2) and new tests, or only the new ones (e.g. for the projection task)? In both cases, I guess we should we enable them from the GitHub action? (they are currently skipped)
And then, in case you want to stick to the new tests only, I suggest to remove the old ones - so that you'll get an overview of whether you are missing out a lot of coverage or not.

@lorenzocerrone
Copy link
Collaborator Author

I have started refactoring the task tests (see the tests/task_v2). It would be great if you could go over it and look if it all looks alright for you.

Quick generic question: will you include both old (non-tests/tasks_v2) and new tests, or only the new ones (e.g. for the projection task)? In both cases, I guess we should we enable them from the GitHub action? (they are currently skipped) And then, in case you want to stick to the new tests only, I suggest to remove the old ones - so that you'll get an overview of whether you are missing out a lot of coverage or not.

My idea would be to keep both of them for now (at least during the refactoring of the first few tasks). We remove the ' task ' tests only when we are confident that the new task_v2 testing offers comparable coverage.

Yes, we need to enable them! Sorry, I overlooked that

@tcompa
Copy link
Collaborator

tcompa commented Apr 15, 2025

@tcompa I have started refactoring the task tests (see the tests/task_v2). It would be great if you could go over it and look if it all looks alright for you.

I added some minor comments, but it definitely looks fine to me.
Let me know if there are specific questions or parts to review in detail.

@tcompa tcompa self-requested a review April 15, 2025 07:55
@lorenzocerrone lorenzocerrone merged commit b63681d into dev-1.5 Apr 16, 2025
16 checks passed
@lorenzocerrone lorenzocerrone deleted the ngio_projection_task branch April 16, 2025 09:18
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