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

Change the output name to something independent from odd/even? #59

Open
rdrighetto opened this issue Mar 25, 2024 · 3 comments
Open

Change the output name to something independent from odd/even? #59

rdrighetto opened this issue Mar 25, 2024 · 3 comments

Comments

@rdrighetto
Copy link
Contributor

I am not sure how best to address this, so I'm raising this issue for brainstorming:

When predicting, the output is currently written with the same file name as the even tomogram supplied:

out_filename = os.path.join(config['output'], os.path.basename(even))

This is confusing to many users, as it seems like the output depends only on the "even" tomogram, when in fact it's the average of the denoised odd and even tomograms.

Potential issues I see:

  1. It's hard to know what a "good" output name looks like, since in principle the input odd and even tomograms can be called anything (and I think this is how it should be)
  2. It's even harder in the case where multiple tomograms are being predicted at once, and we need to identify them uniquely in a way that is easily traceable to the original inputs

One way I see to address this "correctly" is to require explicit output names for every tomogram being predicted, like we already do with the inputs. This is a major'ish API change IMO.

Any thoughts?

@TomCrey
Copy link
Contributor

TomCrey commented Apr 2, 2024

In my opinion, the simplest way would be to append a specific output name: instead of just using the basic name of the even tomogram, you could add a recognizable suffix or prefix to indicate that the output is the result of denoising the odd and even tomograms. For example, replace line 153 as follows:
out_filename = os.path.join(config['output'], "denoised_" + os.path.basename(even))

This would make it clearer that the output is not just based on the even tomogram.

@rdrighetto
Copy link
Contributor Author

Hi @TomCrey!

Thanks for the suggestion!
The problem I see with doing it this way is that it still contains the name of the even tomogram in the file name, so I'm not sure it would solve the confusion.

@TomCrey
Copy link
Contributor

TomCrey commented Apr 2, 2024

Hi @rdrighetto!
I don't think it's the perfect method, but it would reduce the confusion.
Alternatively, we maybe could allow users to specify custom output file names for each predicted tomogram. This would involve modifying the predict_config.json script to accept output file names as output name (but I wouldn't know how to configure this in the json).
While this may represent a significant API change, it could offer the greatest flexibility and clarity for users.

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

No branches or pull requests

2 participants