Abins1.0 minor fixes part2

[dymkowsk]

Description of work.

1. Make sure Abins is using utf8 encoding while reading DFT phonon files.
2. Fix crashing which occurred when there is 'SET' keyword in output CRYSTAL DFT file.

To test:

---

Reviewer

Please comment on the following (full description):

Code Review

• Is the code of an acceptable quality?
• Does the code conform to the coding standards? Is it well structured with small focussed classes/methods/functions?
• Are there unit/system tests in place? Are the unit tests small and test the a class in isolation?
• If there are changes in the release notes then do they describe the changes appropriately?

Functional Tests

• Do changes function as described? Add comments below that describe the tests performed?

• How do the changes handle unexpected situations, e.g. bad input?

• Has the relevant documentation been added/updated?

• Is user-facing documentation written in a user-friendly manner?

• Has developer documentation been updated if required?

• Does everything look good? Comment with the ship it emoji but don't merge. A member of @mantidproject/gatekeepers will take care of it.

[martyngigg]

Thanks for bugfixes. I will take a look.

[martyngigg]

This will change the default encoding for absolutely everyone and it is generally discouraged to do this: http://stackoverflow.com/questions/3828723/why-should-we-not-use-sys-setdefaultencodingutf-8-in-a-py-script

Would the codecs.open function be what we need here?

[dymkowsk]

Ok, I will have a look at codecs.open and change the way I set enconding.

[martyngigg]

@dymkowsk Thanks for the updates. Unfortunately the Windows tests are failing :

10:30:10 ======================================================================
10:30:10 FAIL: test_gamma_crystal (__main__.AbinsLoadCRYSTALTest)
10:30:10 ----------------------------------------------------------------------
10:30:10 Traceback (most recent call last):
10:30:10   File "C:/Jenkins/workspace/pull_requests-win7/scripts/test/AbinsLoadCRYSTALTest.py", line 56, in test_gamma_crystal
10:30:10     self._check(name=self._gamma_crystal)
10:30:10   File "C:/Jenkins/workspace/pull_requests-win7/scripts/test/AbinsLoadCRYSTALTest.py", line 70, in _check
10:30:10     self._check_reader_data(correct_data=correct_data, data=data, filename=name)
10:30:10   File "C:/Jenkins/workspace/pull_requests-win7/scripts/test/AbinsLoadCRYSTALTest.py", line 137, in _check_reader_data
10:30:10     self.assertEqual(correct_data["attributes"]["advanced_parameters"], data["attributes"]["advanced_parameters"])
10:30:10 AssertionError: u'ddc697f699190cac09d189a0f8a39d7fbc5a1835d003a41030327df3821a61744ad40055bcf2075fa52db2374e6c9d73cbb502d0d46839bace32abf29ecdf70c' != '268fb3b2358eb7424d507ec2c6aad3ed34f78b0059ca294e5a7c04c33abc1f2422d87825fd4c60b767b0d77d7604761ce4ff6993185c4a34c7b8875753555852'

[dymkowsk]

Ok, I see. I will fix it.

[martyngigg]

@dymkowsk It looks like the same error is still present on Windows, I'm afraid:

======================================================================
01:25:30 FAIL: test_gamma_crystal (__main__.AbinsLoadCRYSTALTest)
01:25:30 ----------------------------------------------------------------------
01:25:30 Traceback (most recent call last):
01:25:30   File "C:/Jenkins/workspace/pull_requests-win7/scripts/test/AbinsLoadCRYSTALTest.py", line 56, in test_gamma_crystal
01:25:30     self._check(name=self._gamma_crystal)
01:25:30   File "C:/Jenkins/workspace/pull_requests-win7/scripts/test/AbinsLoadCRYSTALTest.py", line 70, in _check
01:25:30     self._check_reader_data(correct_data=correct_data, data=data, filename=name)
01:25:30   File "C:/Jenkins/workspace/pull_requests-win7/scripts/test/AbinsLoadCRYSTALTest.py", line 137, in _check_reader_data
01:25:30     self.assertEqual(correct_data["attributes"]["advanced_parameters"], data["attributes"]["advanced_parameters"])
01:25:30 AssertionError: u'ddc697f699190cac09d189a0f8a39d7fbc5a1835d003a41030327df3821a61744ad40055bcf2075fa52db2374e6c9d73cbb502d0d46839bace32abf29ecdf70c' != '268fb3b2358eb7424d507ec2c6aad3ed34f78b0059ca294e5a7c04c33abc1f2422d87825fd4c60b767b0d77d7604761ce4ff6993185c4a34c7b8875753555852'

[martyngigg]

I wonder if the hash mismatch is because of this comment in https://docs.python.org/2/library/codecs.html#codecs.open:

"Note Files are always opened in binary mode, even if no binary mode was specified. This is done to avoid data loss due to encodings using 8-bit values. This means that no automatic conversion of '\n' is done on reading and writing."

I presume that would mean the hash on windows would be different?

[dymkowsk]

That's very likely is a source of error. I have created additional set of files for Windows use case. I have changed in the reference files hash of AbinsParameters.py to the one which is given in the error message.

[dymkowsk]

Maybe using io module would be a better choice in my case? For example something like:

with io.open(filename,'rt',encoding='utf8') as f:

so we force it to be treated as a text file.

[martyngigg]

@dymkowsk I think you're right. Also given that open=io.open in Python 3 that indicates that's it's probably the correct choice for new code.

[dymkowsk]

@martyngigg I did what we discussed but still the same error. Stuck a bit with this... Any help with fixing this bug is welcome :).

[dymkowsk]

@martyngigg I think I fixed bug. Let me know if more changes is needed.

[martyngigg]

@dymkowsk Thanks for fixes. I was just looking into what was happening on Windows. I checked out the changes at commit 91a848d when the io.open looked like

io.open(file=filename, mode="rt", encoding=coding, buffering=buf, newline=None)

and the tests worked for me. I get the hash of the AbinsParameters.py file as the expected ddc697f699.... The Python docs suggest that this would be the correct thing to do so I'm a little confused as to why it didn't work on the builder.

[martyngigg]

It also works without the try...except UnicodeDecodeError statement.

I think I know what has happened. Commit 91a848d has a red cross next to it but in fact it's not that commit itself that got built. I checked the link and it the commit that sets newline="" that got built and this is not correct.

[martyngigg]

I have created a patch which can be applied:

cd mantid
curl -L https://gist.githubusercontent.com/martyngigg/74089aff29369e0ab300c1a6c1731a9a/raw/5208ded67961a5e2ef6eec9b05d4310d4dad0a1e/abins_iomodule.patch | git apply

It will also require the "'hash" parameter in the _data.txt file to be updated as it doesn't try the ascii encoding first. I think this should be general now.

[dymkowsk]

@martyngigg thanks for patch. I will soon push changes.

[martyngigg]

Oops, I forgot to run the changes through flake8. It needs a newline removing and you can probably remove the h = and just return directly from the line above (a relic of testing): http://builds.mantidproject.org/job/pull_requests-flake8/1952/warnings6Result/package.249446497/source.3686391886509914023/#440

[martyngigg]

Excellent. This looks good to go now.

[AndreiSavici]

@dymkowsk I think this ABINS suite needs a lot of documentation. What is the physics behind it? If anyone want to use it or modify it, they need to know what the code is supposed to be doing. A lot of it is in the requirements document in the original ABINS pull request. Create a Concepts page. Here are a few examples:

• http://docs.mantidproject.org/nightly/concepts/SymmetryGroups.html
• http://docs.mantidproject.org/nightly/concepts/MDNorm.html

[dymkowsk]

@AndreiSavici thank you for your feedback about documentation. I will soon submit new pull request with Concepts page.