search

Chemellia / ChemistryFeaturization.jl

Interface package for featurizing atomic structures
https://chemistryfeaturization.chemellia.org/dev/
MIT License
41 stars 14 forks source link

src: pmg_graphs: fix return value issue, and add new test #52

Closed thazhemadam closed 3 years ago

thazhemadam commented 3 years ago

Consider the case, where build_graphs_batches() is called and there already exists an output_folder, and overwrite = false.

In this case, the build_graph() function isn't called (as expected), but the existing graphs aren't returned to the caller; which is not the expected behaviour.

Also, add a test which checks for this condition.

Signed-off-by: Anant Thazhemadam anant.thazhemadam@gmail.com

thazhemadam commented 3 years ago

If anyone comes up with more "edge cases" that aren't satisfied by the overwrite option (or overwrite in conjunction with some other option), do let me know. 🙂

thazhemadam commented 3 years ago

@rkurchin please don't merge this for a while. I opened this as a PR mainly so you could review more easily. I'll soon be doing a more detailed analysis on what other combination may need to be accounted for, and might come up with a few more test cases.

thazhemadam commented 3 years ago

@rkurchin I couldn't really come up with any more edge cases for which this fix wouldn't work. If you can think of any, do let me know. Otherwise, this should be good to merge.