adding metadata dictionary to cjsonwriter #1148
Conversation
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## master #1148 +/- ##
==========================================
+ Coverage 87.79% 87.80% +0.01%
==========================================
Files 64 69 +5
Lines 13435 13535 +100
==========================================
+ Hits 11795 11885 +90
- Misses 1640 1650 +10
☔ View full report in Codecov by Sentry. |
|
This looks fine to me, but we (probably long ago) have veered off the CJSON format. I used this snippet on the result of Unless we can do some sort of standardization with Marcus (who is definitely still using modified CJSON) and Geoff (who is reworking spectroscopy representation among other things I don't recall), we should have even just a comment in docs that this is "cclib-extended Chemical JSON". |
|
Let's talk over Slack or on the Avogadro forum. I think adding the metadata block is a good idea. I'm not sure if Marcus is doing anything with CJSON right now, it seems as if Brookhaven is keeping him occupied, as well as Tomviz. |
|
I'd certainly like to see something in which reading in the metadata will help configure the input generator GUI (e.g., load an MP2 calculation, and it shows an MP2 calculation by default). |
|
Here's a more recent reference point on the I added some support for But IIRC I didn't add anything on the Avogadro side to parse it yet. Suggestions welcome .. in particular, I'd like to add total charge and spin multiplicity soon, probably under "properties" to properly roundtrip files (e.g., for bond perception, etc.) |
|
I'm very open to standardization, so there can be something relatively well defined when Avogadro 2.0 is released. One relevant example from the has: This seems like an extremely good idea to enable users to "round trip" calculations. A few other useful metadata for the
The "task" key is obviously not in
I'm also open to adding more "task" options. These all seem like they'd be useful to pull out into the "inputParameters" key.. to be used by the Python input generator scripts. The other |
|
I went through the pull requests (#1143, #1142, #1141) and collected the data that they would want to introduce into the cjson, initially proposed as additions to a metadata section. As @ghutchis mentioned, many of these would fit in inputParameters field. There are a few that I am unclear on where they should be placed, I've indicated this with (?) following the data. Specifically, information about hardware related information. basis_set (already exists inputParameters) |
|
Most of these seem useful for We also discussed that "extensions" were probably useful / needed for specific program options. (For example, I'm not sure that Grid5 from Orca corresponds to anything specific in other programs.) Let me clean up the CJSON repo, post the draft schema and I'll submit a patch here based on the I think my only question is the |
|
This sounds like a plan, thank you. I'll work on placing these keywords in the right place then. I do agree that run_date does fit best in metadata. |
8ad3d6c to
a077cf7
Compare
a077cf7 to
f50ccda
Compare
cclib/io/cjsonwriter.py
Outdated
| @@ -123,6 +123,28 @@ def as_dict(self): | |||
| cjson_dict['atoms']['elements']['atom count'] = len(self.ccdata.atomnos) | |||
| cjson_dict['atoms']['elements']['heavy atom count'] = len([x for x in self.ccdata.atomnos if x > 1]) | |||
|
|
|||
| if hasattr(self.ccdata, 'metadata'): | |||
| cjson_dict['inputParameters'] = dict() | |||
| if 'run_date' in self.ccdata.metadata: | |||
There was a problem hiding this comment.
Can this if/else block be improved by using a mapping and loop over that?
e53866e to
b716e11
Compare
0d95982 to
d30d0f0
Compare
There was a problem hiding this comment.
I'm doing something like this to compare:
#!/bin/bash
set -euo pipefail
for example in dvb_sp_un dvb_gopt; do
outfile=/home/eric/development/cclib_berquist/data/QChem/basicQChem5.4/${example}.out
# cclib writer
ccwrite cjson ${outfile} || ret=$?
if [[ $ret -eq 0 ]]; then
< ${example}.cjson jq --indent 4 . > ${example}.cclib.cjson
fi
if [[ -f ${example}.cjson ]]; then
rm ${example}.cjson
fi
# Avogadro writer
python /home/eric/development/forks/python/avogadro-cclib/cclibScript.py --read < ${outfile} | jq --indent 4 . > ${example}.avogadro.cjson
done
cclib/io/cjsonwriter.py
Outdated
| cjson_dict['inputParameters'] = dict() | ||
| cclib_to_json_keys = {'theory':'methods'} | ||
| for i in ['run_date','basis_set','theory','dispersion','functional','grid','memory','processors','qm_program']: | ||
| if i in self.ccdata.metadata: |
| cjson = cclib.io.cjsonwriter.CJSON(data).generate_repr() | ||
|
|
||
| json_data = json.loads(cjson) | ||
| self.assertTrue("run_date" in json_data["metadata"]) |
There was a problem hiding this comment.
Not your fault, but when you rebase this PR, can you change these to
| self.assertTrue("run_date" in json_data["metadata"]) | |
| assert "run_date" in json_data["metadata"] |
There was a problem hiding this comment.
Can we update the asserts and then this can be merged IMO
|
Hi No pressure just bumping this. Have you already addressed Eric's review locally? |
d30d0f0 to
a7f2547
Compare
Here is a quick addition to the cjson writer which would allow the metadata dictionary to be output as its own metadata object. This is being discussed as a desired feature in both #1105 and #1143. I haven't used cjson in my own work so this solution may not be in the right format. Just wanted to discuss a possible approach to incorporating this.