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

Issue 1183 fast cif parsing #1188

Closed

Conversation

ltalirz
Copy link
Member

@ltalirz ltalirz commented Feb 23, 2018

Fix #1183

This adds a scan_type attribute to CifData.
Using scan_type='flex' uses C-routines of PyCifRW and is several times faster than the python-based implementation for large CIF files.

Example:

from aiida.orm.data.cif import CifData
a = CifData('/path/to/file.cif', scan_type='flex')
  • add cif.py to linter (1st commit)
  • add scan_type (2nd commit)
  • add test to check that results are identical (2nd commit)

formatting + fixing minor issues
Example:
from aiida.orm.data.cif import CifData

a = CifData('/path/to/file.cif', scan_type='flex')

Also add test to check that results are identical.
@ltalirz
Copy link
Member Author

ltalirz commented Feb 23, 2018

@giovannipizzi
In order to have the default value of scan_type properly set, I had to both extend _set_defauls
and make scan_type a property, which I find rather ugly (if I only provide _set_defaults, I have no control over the order in which the set_ methods are called and I get an error because set_scan_type is called too late).

Suggestions for improvement welcome.

default2 = CifData(file=f.name, scan_type='default')
self.assertEquals(default._prepare_cif(), default2._prepare_cif())

flex = CifData(file=f.name, scan_type='flex')
Copy link
Member

Choose a reason for hiding this comment

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

Is there a way to check that it is actually using flex as a scan type?
Maybe checking the attribute, but maybe it is not even enough, in case the parsing happens before the scan_type is read? It depends on whether the parsing occurs at init or at _prepare_cif.

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, it actually isn't using flex. I will put the default values in the constructor of CifData instead.

@ltalirz
Copy link
Member Author

ltalirz commented Feb 23, 2018

It's still not quite correct...
I will simply use the constructor as regular people do ;-)

@giovannipizzi
Copy link
Member

I'm not sure I understand what happens when you don't use the property. Can't you use self.get_attr('scan_type') instead of self.scan_type?

@ltalirz
Copy link
Member Author

ltalirz commented Feb 23, 2018

Closing this in favor of a new PR including also #1186

@ltalirz ltalirz closed this Feb 23, 2018
@ltalirz ltalirz deleted the issue_1183_fast_cif_parsing branch April 12, 2018 14:16
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