Added cjson.py which can be used to read cjson #1360
Conversation
|
Thanks for opening this pull request! Please check out our contributing guidelines and check for the automated tests. |
adityaomar3
force-pushed
the
manipulating-cjson
branch
from
ffe95a8
to
5ca715d
Compare
adityaomar3
force-pushed
the
manipulating-cjson
branch
3 times, most recently
from
de0728b
to
9e18e16
Compare
|
currently it will work as I will try to make it more simpler by day so that we dont need to create class object and will work simply like |
|
Here are the build results |
|
@ghutchis Sir, can you please review it? |
.vscode/settings.json
Outdated
| @@ -0,0 +1,6 @@ | |||
| { | |||
| "cSpell.words": [ | |||
There was a problem hiding this comment.
Please do not commit .vscode directories. See gitignore - for example https://github.com/github/gitignore/blob/main/Python.gitignore
There was a problem hiding this comment.
Yeah! Sure, I was also thinking to add these files to .gitignore.
There was a problem hiding this comment.
Same - we do not want to commit .pyc files, which should be blocked by the .gitignore example above.
python/avogadro/cjson.py
Outdated
| py_dict_data = json.load(cjsonFile) | ||
| return py_dict_data | ||
| def __to_cjson(self, element_coords): | ||
| cjson_dict = {"element-coordinates" :element_coords} |
There was a problem hiding this comment.
json.load() is use to read data from cjson/json file to python file, json file is read as dictionary data in python.
When converting back to json I have created list of asked data which will be converted to dictionary data by
cjson_dict = {"element-coordinates" :element_coords} statement. Then we can have an appropriate json format to be dumped.
python/avogadro/cjson.py
Outdated
| import json | ||
| class Cjson: | ||
| def __init__(self): | ||
| print("cjson configured successfully start your operations") |
There was a problem hiding this comment.
No, please do not add debugging statements in a pull request.
python/avogadro/cjson.py
Outdated
| coords = data["atoms"]["coords"]["3d"] | ||
| elements = data["atoms"]["elements"]["number"] | ||
| element_coords = [(*coords[i*3:(i+1)*3], elements[i]) for i in range(0, int(len(coords) / 3))] | ||
| print(type(element_coords)) |
There was a problem hiding this comment.
Again, please do not add debugging statements in pull requests.
There was a problem hiding this comment.
Oops!Sorry I forgot to remove that, will rectify this
|
Sorry, I'm not sure why my review comments were stuck as "pending" - I was wondering when you'd make the changes .. but you didn't get the comments in the first place. |
No problem, sometimes technical issues are there,I will rectify all mistakes shortly. |
adityaomar3
force-pushed
the
manipulating-cjson
branch
4 times, most recently
from
4371136
to
5077326
Compare
|
@ghutchis I have done the changes as told. |
| '''It converts python dictionaries to CJson format''' | ||
| cjsonData = json.dumps(cjson_dict_file, indent=4) | ||
| return (cjsonData) | ||
| def get_atoms_coords(self,filePath): |
There was a problem hiding this comment.
I think we want both a method to get an array of coordinates and set the data["atoms"]["coords"]["3d"] from an array of (-1, 3) coordinates.
If you want to add this method (which returns coordinates + atomic number), I'd want to also see:
- get_atoms_coordinates() - this method
- set_atoms_coordinates() - setting the data[] from an array of coordinates and elements
- get_elements() - simply returning the elements from
data["atoms"]["elements"]["number"] - set_elements() - setting the elements
- get_coordinates() - just return the coordinates
- set_coordinates() - just set the coordinates
There was a problem hiding this comment.
Ok, I will add all these functions shortly.
There was a problem hiding this comment.
@ghutchis can you please help me with the format for input array of coordinates and elements for set_atoms_coordinates.
| coords = data["atoms"]["coords"]["3d"] | ||
| elements = data["atoms"]["elements"]["number"] | ||
| element_coords = [(*coords[i*3:(i+1)*3], elements[i]) for i in range(0, int(len(coords) / 3))] | ||
| cjson_dict = {"element-coordinates" :element_coords} |
There was a problem hiding this comment.
Why? Shouldn't you just return the element_coords array?
There was a problem hiding this comment.
Ok! I got this, actually we can return the array too, but earlier in the you told (#712 (comment) point 2) to return the cjson so I have to make it into dictionary element as Cjson interconversion is python dictionaries.
I have a idea that I should return the array and make private to_cjson() fun public which would take array or list as input and convert it into cjson
python/avogadro/cjson.py
Outdated
| """ | ||
| def __init__(self): | ||
| pass | ||
| def __open_file(self, filePath): |
There was a problem hiding this comment.
Generally when we're creating modules, we want symmetry in names. So if you're creating an __open_file() method, there should be a save version. So if you want __to_cjson then you want __from_cjson.
Or what if a user has a string (e.g. from a URL or from another source)?
So you might want to check if filePath can be considered a file path and if not, try loading it as a json string.
There was a problem hiding this comment.
Sure! I will keep this in mind from now on.
I will check for strings and links as you told
currently It was for the filepath of file present in the system
|
Here are the build results |
adityaomar3
force-pushed
the
manipulating-cjson
branch
3 times, most recently
from
2ee566c
to
8e9a7bd
Compare
…s get_atoms_coords which will get the co-ordinates of all atoms Signed-off-by: aditya <[email protected]> Signed-off-by: Aditya Omar <[email protected]>
Signed-off-by: aditya <[email protected]> Signed-off-by: Aditya Omar <[email protected]>
Signed-off-by: Aditya Omar <[email protected]>
Signed-off-by: Aditya Omar <[email protected]>
…resent Signed-off-by: Aditya Omar <[email protected]>
Signed-off-by: Aditya Omar <[email protected]>
Signed-off-by: Aditya Omar <[email protected]>
adityaomar3
force-pushed
the
manipulating-cjson
branch
from
8e9a7bd
to
3b81c7e
Compare
|
@ghutchis please review the functions added Currently the file is present in the system only, and accessed by the script by passing its path. |
|
Here are the build results |
|
Congrats on merging your first pull request! 🎉 Thanks for making Avogadro better for everyone! |
|
Okay, I'm going to merge this and tweak it a bit. Thanks. |
|
Thankyou sir! This file still needs to be checked on function |
|
Here are the build results |
current method added is get_atoms_coords which will get the co-ordinates of all atoms this aims to solve #712
Developer Certificate of Origin
Version 1.1
Copyright (C) 2004, 2006 The Linux Foundation and its contributors.
1 Letterman Drive
Suite D4700
San Francisco, CA, 94129
Everyone is permitted to copy and distribute verbatim copies of this
license document, but changing it is not allowed.
Developer's Certificate of Origin 1.1
By making a contribution to this project, I certify that:
(a) The contribution was created in whole or in part by me and I
have the right to submit it under the open source license
indicated in the file; or
(b) The contribution is based upon previous work that, to the best
of my knowledge, is covered under an appropriate open source
license and I have the right under that license to submit that
work with modifications, whether created in whole or in part
by me, under the same open source license (unless I am
permitted to submit under a different license), as indicated
in the file; or
(c) The contribution was provided directly to me by some other
person who certified (a), (b) or (c) and I have not modified
it.
(d) I understand and agree that this project and the contribution
are public and that a record of the contribution (including all
personal information I submit with it, including my sign-off) is
maintained indefinitely and may be redistributed consistent with
this project or the open source license(s) involved.