Modify resclust outputs #330
Conversation
src/arctic3d/modules/clustering.py
Outdated
| if (strcl := str(clusters[cl])) not in cl_dict.keys(): | ||
| cl_dict[strcl] = [ligands[cl]] |
There was a problem hiding this comment.
This will change the type of the key from integer to string and can have breaking consequences, why not use integers?
There was a problem hiding this comment.
This is the point of the PR, integer as keys cannot be loaded using the json library.
There was a problem hiding this comment.
Yes this is the default behaviour of json.load, casting the type into it needs you to reclass a Decoder
There was a problem hiding this comment.
I don't agree with this and the code looks very messy.
json.loads cannot read keys as integers because this is the default json format.
This PR adds an anti-pattern solution as convenience and I don't think this is a good idea, what needs to be done is to re-think the output data structure and adopt a standard that is compliant with the default format.
Arguably changing 1 to "1" does achieve that but it relies heavy on python dynamic type casting and less on the output data formalism, which can be very prone to errors in the future.
up to you @mgiulini
Hi there, sorry for the late reply. For what I see, adding the can be useful and is in line with the other output files. As for the json, I agree that that modification is not in the purpose of the project and of this CLI in particular. Maybe @VGPReys you can load the cluster data from the |
Ok then let's just stick to the textual version in |
removing string casting
removing json output file
removing json output file test
removing string version on keys in test_get_cl_dict()
To solve the two improvments addressed in issue #327:
Dict keys have been casted to string.
Tests data have been modified too.
While printed in
log, dict is converted to string, and default'character are replaced to", making sure thejsonlibrary will be able toloads(string)the data.While providing the
--output=path/to/resultsargument, the user can now add a path where to generate the Clutering results.An additional test function was added to make sure the output is indeeed created while providing the argument.