Skip to content

reusing flats/darks from a different scan #569

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 11 commits into from
Apr 25, 2025
Merged

reusing flats/darks from a different scan #569

merged 11 commits into from
Apr 25, 2025

Conversation

dkazanc
Copy link
Collaborator

@dkazanc dkazanc commented Apr 11, 2025

Enables reusing flats/darks coming from a different scan by providing an additional image_key_path parameter.

Checklist

  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • I have made corresponding changes to the documentation

@dkazanc
Copy link
Collaborator Author

dkazanc commented Apr 11, 2025

@yousefmoazzam I've got few test failures (6) in this branch that are related to the output filename changes. Specifically the error is:

AssertionError: assert 2 == 1
 +  where 2 = len(['/tmp/pytest-of-algol/pytest-0/test_recon_method_output_filen3/some-recon.h5', '/tmp/pytest-of-algol/pytest-0/test_recon_method_output_filen3/some-recon.h5-3056533504-35197.lock'])
 

Have you seen this one? I reckon it counts the lock file being temporarily there.

@yousefmoazzam
Copy link
Collaborator

@yousefmoazzam I've got few test failures (6) in this branch that are related to the output filename changes. Specifically the error is:

AssertionError: assert 2 == 1
 +  where 2 = len(['/tmp/pytest-of-algol/pytest-0/test_recon_method_output_filen3/some-recon.h5', '/tmp/pytest-of-algol/pytest-0/test_recon_method_output_filen3/some-recon.h5-3056533504-35197.lock'])
 

Have you seen this one? I reckon it counts the lock file being temporarily there.

I have indeed seen this when running tests locally, but not in recent CI jobs.

If CI were to also report the error then I'd be inclined to think that a dependency's version has been updated which breaks things, but that isn't the case. So unfortunately I'm not yet sure what to make of it.

Copy link
Collaborator

@yousefmoazzam yousefmoazzam left a comment

Choose a reason for hiding this comment

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

Just one comment for now about clarifying types, will keep looking

Copy link
Collaborator

@yousefmoazzam yousefmoazzam left a comment

Choose a reason for hiding this comment

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

Looks good overall, a few comments about making the new test a bit clearer, and some bits about docs.

dkazanc and others added 7 commits April 25, 2025 12:20
Co-authored-by: Yousef Moazzam <yousefmoazzam@users.noreply.github.com>
Co-authored-by: Yousef Moazzam <yousefmoazzam@users.noreply.github.com>
Co-authored-by: Yousef Moazzam <yousefmoazzam@users.noreply.github.com>
Co-authored-by: Yousef Moazzam <yousefmoazzam@users.noreply.github.com>
Co-authored-by: Yousef Moazzam <yousefmoazzam@users.noreply.github.com>
@dkazanc dkazanc merged commit cdcb31a into main Apr 25, 2025
6 of 7 checks passed
@dkazanc dkazanc deleted the external_flats branch April 25, 2025 12:06
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