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

Downgrade remark type number invalid error #124

Merged
merged 2 commits into from
May 16, 2024

Conversation

y1zhou
Copy link
Contributor

@y1zhou y1zhou commented May 15, 2024

Fixes #123 and adds a test PDB file containing such REMARK records.

Many software-generated model files, e.g. Rosetta PDB files, put arbitrary remark numbers in the file. Downgrading the error level would allow pdbtbx to deal with such files.

resolves douweschulte#123
@y1zhou
Copy link
Contributor Author

y1zhou commented May 15, 2024

@douweschulte It seems the ReadOptions hasn't made its way into the core parser yet? The open_with_options function only passes ReadOptions.level into the open_pdb / open_mmcif function and ignores all other fields.

pub(in crate::read) fn open_with_options(
filename: impl AsRef<str>,
options: &ReadOptions,
) -> ReadResult {
if check_extension(&filename, "pdb") {
open_pdb(filename, options.level)
} else if check_extension(&filename, "cif") {
open_mmcif(filename, options.level)
} else {
Err(vec![PDBError::new(
ErrorLevel::BreakingError,
"Incorrect extension",
"Could not determine the type of the given file, make it .pdb or .cif",
Context::show(filename.as_ref()),
)])
}
}

To implement the only_atomic_data read option we'd need corresponding open_pdb_with_options and open_mmcif_with_options as well, right? I assume you don't want to modify the original signatures directly for backward compatibility.

@douweschulte
Copy link
Owner

Yes something like that has to be added. On not changing the API, you can change the 'core' methods (meaning the one that actually does the parsing), but make sure to keep this one private and keep the current open functions as calls to the core functions with the default ReadOptions for example.

@OWissett
Copy link
Collaborator

I have created another issue for finishing my implementation of the ReadOptions. #125

Sorry for leaving it half done, I got stuck with some other PhD work.

@y1zhou
Copy link
Contributor Author

y1zhou commented May 15, 2024

Cool I'll try to add in the functions that parse ReadOptions.

Sorry for leaving it half done, I got stuck with some other PhD work.

No worries we are all aware how stressful that can be :)

@OWissett OWissett added this to the v0.12.0 milestone May 15, 2024
Copy link
Owner

@douweschulte douweschulte left a comment

Choose a reason for hiding this comment

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

Perfect!! I am fine with merging right away and doing the only_load_atomic together with #125 in another PR or doing that work right here whatever works best for you.

@OWissett OWissett marked this pull request as ready for review May 15, 2024 17:43
@y1zhou
Copy link
Contributor Author

y1zhou commented May 15, 2024

Perfect!! I am fine with merging right away and doing the only_load_atomic together with #125 in another PR or doing that work right here whatever works best for you.

Yes taking that in another PR seems more reasonable. Will send as more work is done!

@douweschulte douweschulte merged commit b4afa9a into douweschulte:master May 16, 2024
4 checks passed
@y1zhou y1zhou deleted the atomic-only branch May 16, 2024 06:50
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.

Read option to only parse ATOM records
3 participants