ISISNeutronMuon / MDANSE

MDANSE: Molecular Dynamics Analysis for Neutron Scattering Experiments
https://www.isis.stfc.ac.uk/Pages/MDANSEproject.aspx
GNU General Public License v3.0
21 stars 5 forks source link

ENHANCEMENT: Change default name of output files #41

Closed RastislavTuranyi closed 2 years ago

RastislavTuranyi commented 2 years ago

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 commented 2 years ago

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

eurydice76 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 _, e.g. NaF_castep) and place it into the same directory as the currently inputted trajectory. What do you think?

RastislavTuranyi commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

I implemented in 303bbc53b180a7efae46e93219cce6ca42c34e2e 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 commented 2 years ago

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