Skip to content

Conversation

@laldoroty
Copy link
Collaborator

Checked both keep_intermediate true and keep_intermediate false. Works.

laldoroty added 2 commits June 5, 2025 08:58
…itionally save the original images, sky-subtracted images, detection masks, and input (original) PSFs to dia_out_dir.
@laldoroty laldoroty requested review from rknop and wmwv June 5, 2025 20:16
@laldoroty laldoroty linked an issue Jun 5, 2025 that may be closed by this pull request
root and others added 2 commits June 5, 2025 20:20
Copy link
Collaborator

@wmwv wmwv left a comment

Choose a reason for hiding this comment

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

I feel like I only half-understand the logic flow here of what determines when the images get written out. I think I expected more something that would wrap each step and then optionally write things out. My understanding (incomplete) of this PR is that it gathers all of the data files, keeps them in memory, and then writes them out again. That sounds like (a) a lot of memory usage; and (b) frustrating if the code crashes before it reaches the point where it writes out the intermediate files.

]
}
# Write the aligned images
for key in write_filepaths.keys():
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does waiting until here mean that all the steps have to have been successful to get any of the images to be written out?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Only for the portions on GPU.

Copy link
Collaborator

@wmwv wmwv left a comment

Choose a reason for hiding this comment

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

Add a test that exercises these functionality. Somewhere in this repo or in examples at least to capture it.

laldoroty added 3 commits June 5, 2025 13:31
…te perlmutter example interactive_podman.sh file to match. Address some review comments.
Copy link
Collaborator

@wmwv wmwv left a comment

Choose a reason for hiding this comment

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

A little philosophy here, but I'd prefer the get_psf function to be simpler and not have to know about the config object and that rather the thing calling get_psf explicitly set save_file=True in the call if self.keep_itermediate is set.

I'm open to discussions, but I'd like to not end up where all functions have few arguments but have lots of implicit dependencies on the config object even having an attribute defined.

@wmwv
Copy link
Collaborator

wmwv commented Jun 5, 2025

And no need to actually edit the commit, but

"[Move intermediate file save location to /pscratch/sd/l/laldoroty..."

should be

"Move intermediate file save location to $SCRATCH/phrosty_intermediate"

I was worried at first that you had hardcoded something.

@laldoroty
Copy link
Collaborator Author

And no need to actually edit the commit, but

"[Move intermediate file save location to /pscratch/sd/l/laldoroty..."

should be

"Move intermediate file save location to $SCRATCH/phrosty_intermediate"

I was worried at first that you had hardcoded something.

...I have no memory of typing this, where is it?

@laldoroty laldoroty requested a review from wmwv June 6, 2025 00:19
@rknop
Copy link
Collaborator

rknop commented Jun 6, 2025

A little philosophy here, but I'd prefer the get_psf function to be simpler and not have to know about the config object and that rather the thing calling get_psf explicitly set save_file=True in the call if self.keep_itermediate is set.

I'm open to discussions, but I'd like to not end up where all functions have few arguments but have lots of implicit dependencies on the config object even having an attribute defined.

So.... in this specific case, get_psf may well need to know about the config object, because one of the things that gets configed is the type of PSF we're using. It's cleaner to have that ugliness buried inside a get_psf function than to have something outside know about that and pass a type variable to get_psf. (But, perhaps, then actual ugliness should be even deeper, in snappl. I would have to look at what's happening here to have an informed opinion.)

@wmwv
Copy link
Collaborator

wmwv commented Jun 6, 2025

And no need to actually edit the commit, but
"[Move intermediate file save location to /pscratch/sd/l/laldoroty..."
should be
"Move intermediate file save location to $SCRATCH/phrosty_intermediate"
I was worried at first that you had hardcoded something.

...I have no memory of typing this, where is it?

This was in the git commit message.

@wmwv
Copy link
Collaborator

wmwv commented Jun 6, 2025

A little philosophy here, but I'd prefer the get_psf function to be simpler and not have to know about the config object and that rather the thing calling get_psf explicitly set save_file=True in the call if self.keep_itermediate is set.
I'm open to discussions, but I'd like to not end up where all functions have few arguments but have lots of implicit dependencies on the config object even having an attribute defined.

So.... in this specific case, get_psf may well need to know about the config object, because one of the things that gets configed is the type of PSF we're using. It's cleaner to have that ugliness buried inside a get_psf function than to have something outside know about that and pass a type variable to get_psf. (But, perhaps, then actual ugliness should be even deeper, in snappl. I would have to look at what's happening here to have an informed opinion.)

I'd like to separate things like "detailed configuration of PSF" from pipeline operational choices like whether or not to save an intermediate image. The former seems like a good reason to pass an object with a bunch of stuff, the later seems like an something that is a different kind of config .

Copy link
Collaborator

@wmwv wmwv left a comment

Choose a reason for hiding this comment

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

I'm fine with this.

@laldoroty
Copy link
Collaborator Author

And no need to actually edit the commit, but
"[Move intermediate file save location to /pscratch/sd/l/laldoroty..."
should be
"Move intermediate file save location to $SCRATCH/phrosty_intermediate"
I was worried at first that you had hardcoded something.

...I have no memory of typing this, where is it?

This was in the git commit message.

That's so weird. I definitely typed $SCRATCH/phrosty_intermediate. It must have interpreted $SCRATCH as the above hardcoded path.

@laldoroty laldoroty merged commit e7d0c33 into main Jun 6, 2025
3 checks passed
github-actions bot pushed a commit that referenced this pull request Jun 6, 2025
@wmwv
Copy link
Collaborator

wmwv commented Jun 6, 2025

That's so weird. I definitely typed $SCRATCH/phrosty_intermediate. It must have interpreted $SCRATCH as the above hardcoded path.

Yes, if you directly typed that on the command-line, e.g., git commit -m "Move $SCRATCH/phrosty_intermediate" without escaping the $, the shell would have expanded $SCRATCH into the full path before passing the argument list to git.

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.

Add option to save intermediate files

4 participants