Skip to content
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

Merged
merged 7 commits into from
Oct 25, 2023

Conversation

adityaomar3
Copy link
Contributor

@adityaomar3 adityaomar3 commented Oct 4, 2023

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.

@welcome
Copy link

welcome bot commented Oct 4, 2023

Thanks for opening this pull request! Please check out our contributing guidelines and check for the automated tests.

@adityaomar3 adityaomar3 force-pushed the manipulating-cjson branch 3 times, most recently from de0728b to 9e18e16 Compare October 4, 2023 20:39
@adityaomar3
Copy link
Contributor Author

adityaomar3 commented Oct 5, 2023

currently it will work as
from avogadro import cjson as cj cj.Cjson().get_atoms_coords("file_path")

I will try to make it more simpler by day so that we dont need to create class object and will work simply like
from avogadro import cjson as cj cj.get_atoms_coords("file_path")

@github-actions
Copy link
Contributor

github-actions bot commented Oct 5, 2023

Here are the build results
Avogadro2.AppImage
Win64.exe
Artifacts will only be retained for 90 days.

@adityaomar3
Copy link
Contributor Author

adityaomar3 commented Oct 9, 2023

@ghutchis Sir, can you please review it?
Tested on https://github.com/OpenChemistry/avogadrolibs/files/12569568/h2o.cjson.gz file

@@ -0,0 +1,6 @@
{
"cSpell.words": [
Copy link
Member

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

Copy link
Contributor Author

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.

Copy link
Member

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 Show resolved Hide resolved
py_dict_data = json.load(cjsonFile)
return py_dict_data
def __to_cjson(self, element_coords):
cjson_dict = {"element-coordinates" :element_coords}
Copy link
Member

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?

Copy link
Contributor Author

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.

import json
class Cjson:
def __init__(self):
print("cjson configured successfully start your operations")
Copy link
Member

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.

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))
Copy link
Member

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.

Copy link
Contributor Author

@adityaomar3 adityaomar3 Oct 10, 2023

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, no .pyc files.

@ghutchis
Copy link
Member

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.

@adityaomar3
Copy link
Contributor Author

adityaomar3 commented Oct 10, 2023

not

No problem, sometimes technical issues are there,I will rectify all mistakes shortly.

@adityaomar3 adityaomar3 force-pushed the manipulating-cjson branch 4 times, most recently from 4371136 to 5077326 Compare October 11, 2023 21:42
@adityaomar3 adityaomar3 requested a review from ghutchis October 11, 2023 22:21
@adityaomar3
Copy link
Contributor Author

@ghutchis I have done the changes as told.
Whenever you get time, please review it again.

'''It converts python dictionaries to CJson format'''
cjsonData = json.dumps(cjson_dict_file, indent=4)
return (cjsonData)
def get_atoms_coords(self,filePath):
Copy link
Member

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

Copy link
Contributor Author

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.

Copy link
Contributor Author

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}
Copy link
Member

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?

Copy link
Contributor Author

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

"""
def __init__(self):
pass
def __open_file(self, filePath):
Copy link
Member

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.

Copy link
Contributor Author

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

@github-actions
Copy link
Contributor

Here are the build results
Avogadro2.AppImage
macOS.dmg
Win64.exe
Artifacts will only be retained for 90 days.

@adityaomar3 adityaomar3 force-pushed the manipulating-cjson branch 3 times, most recently from 2ee566c to 8e9a7bd Compare October 21, 2023 12:50
@adityaomar3
Copy link
Contributor Author

adityaomar3 commented Oct 21, 2023

@ghutchis please review the functions added
Also, currently the set_atoms_coordinates and set_coordinates sort of work similar, I need a bit help in format of input array for both functions(a sample) to distinguish their functionality

Currently the file is present in the system only, and accessed by the script by passing its path.
We can also parse json string, just minute changes if you say so.

@adityaomar3 adityaomar3 requested a review from ghutchis October 23, 2023 19:35
@github-actions
Copy link
Contributor

Here are the build results
Avogadro2.AppImage
macOS.dmg
Win64.exe
Artifacts will only be retained for 90 days.

@ghutchis ghutchis merged commit 301aa7d into OpenChemistry:master Oct 25, 2023
17 of 18 checks passed
@welcome
Copy link

welcome bot commented Oct 25, 2023

Congrats on merging your first pull request! 🎉 Thanks for making Avogadro better for everyone!

@ghutchis
Copy link
Member

Okay, I'm going to merge this and tweak it a bit. Thanks.

@adityaomar3
Copy link
Contributor Author

Thankyou sir! This file still needs to be checked on function
set_atoms_coordinates.
Will create a new patch for that if necessary

@github-actions
Copy link
Contributor

Here are the build results
Avogadro2.AppImage
macOS.dmg
Win64.exe
Artifacts will only be retained for 90 days.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants