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

ENHANCEMENT: Change default name of output files #41

Closed
RastislavTuranyi opened this issue Mar 4, 2022 · 10 comments · Fixed by #60
Closed

ENHANCEMENT: Change default name of output files #41

RastislavTuranyi opened this issue Mar 4, 2022 · 10 comments · Fixed by #60
Labels
enhancement New feature or request

Comments

@RastislavTuranyi
Copy link
Collaborator

Currently, the default name of output files of all analysis is output_<trajectory> where is the name of the trajectory file. This default name is highly undescriptive, requiring to be changed every time an analysis is performed because if it is not, the file will simply get overwritten. A better name would be <trajectory>_<analysis> where is an abbreviated name of the analysis performed. Even better, MDANSE could also check if the default file name already exists, and if it does, add (1) etc at the end to prevent overwriting.

@RastislavTuranyi RastislavTuranyi added the enhancement New feature or request label Mar 4, 2022
@RastislavTuranyi
Copy link
Collaborator Author

RastislavTuranyi commented Apr 5, 2022

If anyone is interested, I have made some progress on this a while ago; it is in branch 41_descriptive_output_file_names.

@eurydice76
Copy link
Collaborator

Hi Rastislav, I committed an updated version of your algorithm for checking if an output file already exists or not. Basically, I replaced the for:else loop by a while. Indeed, I disliked the fact that the number of file tested was hardcoded (100) that in the else condition a random number was tested. Also the existence checking is now only made at the basename level because anyway at this level we can not know which format the user will select. If you are OK with this, I can create a PR.

@RastislavTuranyi
Copy link
Collaborator Author

I used the for else loop to avoid the edge case where there is a huge number of already existing files (thousands or even more), but if you think that is so unlikely as to be of no concern, then replacing the for loop with a while loop is fine, and I guess filesInDirectory being too large is then also not a problem.

However, the reason why I have not opened a PR yet is that I have been wondering if there was a way to make the output file names for trajectory conversion more relevant. I have been able to at least include the name of the converter in the file name, but that is not much more useful than the default path, $HOME/output_trajectory_conversion, which is completely useless since I have to change the directory and the name every time I do a conversion.

@eurydice76
Copy link
Collaborator

I think that we can reasonably think that the case where thousands or even more already existing files occurs should not be a problem. So I would keep my code.
For the trajectory conversion, it is quite a tricky problem. When the trajectory conversion dialog is opened we do not have so much information at hand to set a smart output file name. I do not see right now how to proceed. We have to think about this.

@RastislavTuranyi
Copy link
Collaborator Author

Perhaps we could add a new button to trajectory converter windows which, when clicked, would generate a meaningful file name (either keeping the name of the inputted trajectory, or <old_name>_<converter_name>, e.g. NaF_castep) and place it into the same directory as the currently inputted trajectory. What do you think?

@RastislavTuranyi
Copy link
Collaborator Author

Actually, a more elegant solution would be to change the output file name when user select a new input file using the browse button (i.e. when they hit submit in the opened window).

@eurydice76
Copy link
Collaborator

eurydice76 commented Apr 8, 2022

no that easy, that means that the OutputFileWidget has to be linked to the InputFileWidget. I remember that I have implemented some kind of widgets dependencies mechanism but I forgot how to use it. I am really not sure it can be done. What is sure is that I dislike the new button approach as it breaks the generality of the job panel generation directly from the job configuration.

@RastislavTuranyi
Copy link
Collaborator Author

Well, if it is not possible to link the two widgets, no matter; I will take look to see if there is a way.

I think there could be a way to add a new button and preserve the generality of generation if we changed the code such that whether the button appears or not would be configured through the settings dictionary inside each job. Either we could create a new widget, or we could modify the OutputFileWidget by passing in a unique keyword argument to OutputFileConfigurator.

@eurydice76
Copy link
Collaborator

eurydice76 commented Apr 8, 2022

I implemented in 303bbc5 a mechanism for guessing the output filename for the converters. It was quite tricky but I think I did it quite nicely. Please review carefully.

@RastislavTuranyi
Copy link
Collaborator Author

You have found a way, thank you! I have opened a PR and added some comments, but overall, this is great!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants