set xsf as default format for structures visualization #1756
Conversation
Codecov Report
@@ Coverage Diff @@
## release_v0.12.2 #1756 +/- ##
===================================================
+ Coverage 54.67% 54.71% +0.04%
===================================================
Files 246 246
Lines 32432 32433 +1
===================================================
+ Hits 17731 17745 +14
+ Misses 14701 14688 -13
Continue to review full report at Codecov.
|
aiida/backends/tests/restapi.py
Outdated
| from aiida.backends.tests.dataclasses import simplify | ||
| node_uuid = self.get_dummy_data()["structuredata"][0]["uuid"] | ||
| url = self.get_url_prefix() + '/structures/' + str( | ||
| node_uuid) + '/content/visualization' |
There was a problem hiding this comment.
This is testing also if the default is XSF.
I think it's better to test explicitly setting the xsf format tin the url.
Also, in general (in the future) probably better not to change the default or not even have one (and make sure already now, in any case, that the frontend always requires a specific format)
| @@ -83,7 +83,7 @@ def __init__(self, **kwargs): | |||
| def get_visualization_data(node, format=None, supercell_factors=[1, 1, 1]): | |||
| """ | |||
| Returns: data in specified format. If format is not specified returns data | |||
| in a format required by chemdoodle to visualize a structure. | |||
| in xsf format in order to visualize the structure with JSmol. | |||
| """ | |||
| response = {} | |||
| response["str_viz_info"] = {} | |||
There was a problem hiding this comment.
Here below, I would do (if we want to have 'xsf' as default):
if format is None:
format = 'xsf' and then let the logic take care of the default.
Then, in the else, raise an exception (now instead it returns 'xsf' even if the string is unknown).
| @@ -95,7 +95,7 @@ def get_visualization_data(node, format=None, supercell_factors=[1, 1, 1]): | |||
| except LicensingException as e: | |||
| response = e.message | |||
|
|
|||
| else: | |||
| elif format == "chemdoodle": | |||
| import numpy as np | |||
There was a problem hiding this comment.
I know this was there before, but can we move this bit of logic and deal with it as with the other formats, instead of doing it specially here?
The way to do it is to add a _prepare_chemdoodle method to the StructureData class (aiida/orm/data/structure.py), see e.g. _prepare_xyz. You can move all the code generation in there.
Then chemdoodle will automatically be found among the formats, and you don't need a special case here.
There was a problem hiding this comment.
just one question: the _prepare_chemdoodle method should return a string (as all others prepare methods) or I can still return the json dictionary?
There was a problem hiding this comment.
Mmm, good point. I think it should return a string.
Maybe you can then return a valid JSON string?
There was a problem hiding this comment.
Ok. I will add a JSON parse in the Javascript to retrieve the correct format to pass to chemdoodle.
There was a problem hiding this comment.
indeed, I didn't think to this.
I still think that the right thing to do is to return a string in AiiDA (as the method is called _exportstring and it might need to be stored on file).
To preserve back-compatibility with people using a REST API at a earlier version, can you put in the client code a test on the type: if it's already a JSON Object, just use it, while if it's a string, parse it first?
| try: | ||
| response["str_viz_info"]["data"] = node._exportstring("xsf")[0] | ||
| response["str_viz_info"]["format"] = "xsf" | ||
| except LicensingException as e: |
There was a problem hiding this comment.
I'm not sure why are checking for LicensingException here, but this was already there, so I'm discussing this with Snehal as well. For now it's ok to keep it.
switched to xsf format as default format in get_visualization_data
added test_xsf_visualization