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

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

Closed
JBGreisman opened this issue Mar 9, 2022 · 3 comments · Fixed by #136
Closed

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

JBGreisman opened this issue Mar 9, 2022 · 3 comments · Fixed by #136
Assignees
Labels
bug Something isn't working documentation Issue related to documentation

Comments

@JBGreisman
Copy link
Member

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.

@JBGreisman JBGreisman added the documentation Issue related to documentation label Mar 9, 2022
@dennisbrookner dennisbrookner self-assigned this Mar 9, 2022
@dennisbrookner dennisbrookner added the bug Something isn't working label Mar 9, 2022
@dennisbrookner
Copy link
Contributor

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

@JBGreisman JBGreisman changed the title rs.read_precognition() supports .ii files rs.read_precognition(): Update docstring and set spacegroup after parsing log Mar 9, 2022
@dennisbrookner
Copy link
Contributor

dennisbrookner commented Mar 9, 2022

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
Copy link
Member Author

JBGreisman commented Mar 9, 2022

Awesome -- thanks! Two small clarifications:

  1. I think the spacegroup is parsed correctly, it is just not set as a DataSet attribute.
  2. 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.

JBGreisman added a commit that referenced this issue Mar 14, 2022
Fixed `read_precognition()` as per #135 and updated tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working documentation Issue related to documentation
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants