`rs.read_precognition()`: Update docstring and set `spacegroup` after parsing log

rs-station / reciprocalspaceship

Tools for exploring reciprocal space

https://rs-station.github.io/reciprocalspaceship/

MIT License

28 stars 12 forks source link

`rs.read_precognition()`: Update docstring and set `spacegroup` after parsing log #135

Closed JBGreisman closed 2 years ago

JBGreisman commented 2 years ago

It came up today that rs.read_precognition() reads .ii files. This isn't explicitly stated in the docstring. It should be made more clear that the io function supports both file types.

dennisbrookner commented 2 years ago

Related to this issue, there's a bug in the logfile parsing of rs.read_precognition() where the spacegroup is parsed from the log file, but never actually assigned to the DataSet. I'll fix both of these in the same PR

dennisbrookner commented 2 years ago

Following additional discussion offline, this PR will

[ ] Clarify in docs that .hkl and .ii files are allowed
[ ] Fix bug in which space group is not parsed from log files
[ ] Clarify in docs that user-supplied space group and/or cell will take precedence over those found in log file
[ ] Add a warning for when user supplies both a space group and/or cell and a log file

JBGreisman commented 2 years ago

Awesome -- thanks! Two small clarifications:

I think the spacegroup is parsed correctly, it is just not set as a DataSet attribute.
I think the warning should only be raised if the user supplies all 3 arguments (sg, cell, and log). I could see a valid use to providing just a spacegroup or a cell if you only want to manually overwrite one attribute. I think the docstring should specify the precedence, but a warning should only be raised when the logfile parsing will not be used.