-
-
Notifications
You must be signed in to change notification settings - Fork 175
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Added cjson.py which can be used to read cjson #1360
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. |
ffe95a8
to
5ca715d
Compare
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah! Sure, I was also thinking to add these files to .gitignore.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand this. Why?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, please do not add debugging statements in pull requests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops!Sorry I forgot to remove that, will rectify this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, no .pyc
files.
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. |
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I will add all these functions shortly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why? Shouldn't you just return the element_coords
array?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
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]>
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.