search

molovol / MoloVol

MoloVol is a free, cross-plattform, scientific software for volume and surface computations of single molecules and crystallographic unit cells.
https://molovol.com
MIT License
22 stars 4 forks source link

Macroscopic volumes and areas calculation #93

Closed rlavendomme closed 3 years ago

rlavendomme commented 3 years ago

Fix issue #90

rlavendomme commented 3 years ago

I noticed that when I try to load the old radii.txt file instead of the elements.txt file, the program crashes. With any other text file I get an error message.

I also noticed the crash with radii.txt The problem was the presence of a line of 4 substrings with only alphabetical substrings: Number Element vdw radius I presume it crashes because it cannot convert the first substring to a number (there is no error catch for that substring). Indeed, the only condition to check if we have an element line is if(substrings.size() == 4) in extractDataFromElemFile()

In the report file, some indents aren't where they should be. I believe the issue is with \t, because its output is unreliable. I've recently written a function that solves this in the command line and I believe it should be applicable here as well.

Strange, I checked all possible case scenarios (option combinations) and got the correct alignement on windows. If spaces are better to align across different OS and you have a function for that, then it's all good.

jmaglic commented 3 years ago

Strange, I checked all possible case scenarios (option combinations) and got the correct alignement on windows. If spaces are better to align across different OS and you have a function for that, then it's all good.

Layout is fixed now, for me at least. Let me know if everything works for you. I even saved a few lines of code. All the layout decisions are now centralised in lambda functions inside model_outputfiles.cpp. It may look a bit confusing but this way any change to the layout immediately changes the whole file.

However, I've noticed another problem: When using the CLI to generate report files, the g/cm^3 values can't be calculated. I believe this is because the molar mass is never calculated when not using the GUI.

rlavendomme commented 3 years ago

Layout is fixed now, for me at least. Let me know if everything works for you. I even saved a few lines of code. All the layout decisions are now centralised in lambda functions inside model_outputfiles.cpp. It may look a bit confusing but this way any change to the layout immediately changes the whole file.

Works nicely. I fixed some wrong volume values displayed in the report (see f3d93bb )

However, I've noticed another problem: When using the CLI to generate report files, the g/cm^3 values can't be calculated. I believe this is because the molar mass is never calculated when not using the GUI.

This is likely because the Ctrl::runCalculation(args) function called by the CLI only extracts radii from elements.txt when setting parameters: _current_calculation->extractRadiusMap(elements_file_path) To fix this, I suppose that the proper way to proceed would be to call extractDataFromElemFile() in Ctrl::runCalculation(args) to store both radii and weight maps. Then set manually the weight map. I wouldn't put the weight map in setParameters() because this is not a parameter available in the GUI unlike all other arguments in setParameters()

jmaglic commented 3 years ago

I wouldn't put the weight map in setParameters() because this is not a parameter available in the GUI unlike all other arguments in setParameters()

Agreed. I use the import function now. The one that imports radius map and weight map. Then, for setParameters() I simply get the radius map and pass it as an argument. This doesn't do anything, but it get's around having to make a new overloaded function with the radius map argument missing.

Only issue left now is to fix the crash. I'll let you know when that's fixed.

jmaglic commented 3 years ago

I believe all issues are fixed now. I found a somewhat tricky bug, that would cause the app to crash whenever a structure contained elements that were not contained in the elements.txt file. That is now fixed. I think the branch is now good to merge.