Handling some nulls in PdbId creation #1020
Conversation
aalhossary
left a comment
aalhossary
left a comment
There was a problem hiding this comment.
It should fix the issue.
I just have a minor comment for easier code readability.
| if(pdbId == null) this.pdbId = null; | ||
| this.pdbId = new PdbId(pdbId); | ||
| if (pdbId == null) | ||
| this.pdbId = null; |
| } | ||
| return new SubstructureIdentifier(new PdbId(pdbId), ranges); | ||
| PdbId pdbIdObj = null; | ||
| if (pdbId != null) { |
There was a problem hiding this comment.
It took me some time to figure it out.
Could it be rewritten using the triple operator (?:), please?
There was a problem hiding this comment.
Good point, now pushed
There was a problem hiding this comment.
I guess should it be (PdbId)null not just null.
Otherwise, the function call would be ambiguous and the compiler would either 1) throw a compile time exception, or 2) go to the first constructor that matches the given signature (SubstructureIdentifier(String pdbId, List<ResidueRange> ranges) in our case) which will try to create a PdbId object out of the passed-in null parameter.
aalhossary
left a comment
aalhossary
left a comment
There was a problem hiding this comment.
I guess should it be (PdbId)null not just null.
Otherwise, the function call would be ambiguous and the compiler would either 1) throw a compile time exception, or 2) go to the first constructor that matches the given signature (SubstructureIdentifier(String pdbId, List<ResidueRange> ranges) in our case) which will try to create a PdbId object out of the passed-in null parameter.
|
True, I've just pushed the commit. Though somehow IntelliJ insists that the casting is redundant, not sure why. |
|
Thanks for this. I also had a branch working on this issue. I think you made the same fixes as me, but I did have some additional tests. |
The core URLIdentifier bug was fixed in biojava#1020, but this improves the test. The parser changes are mostly for logging, and are there to indicate that null PdbId is an expected situation rather than an error.
* Fix CE-Symm bug Fix NullPointer if the structure identifier was missing * Test current handling of identifiers in structures Currently `Structure` has 4 identifiers which may or may not be set depending on the source, file format, and parsing method. These should be revisited and harmonized in the future, but for now I just document them in this test. Summary of results: | Format | With ID | Parse Method | PdbId | Identifier | Name | StructureIdentifier | |--------|---------|-----------------|-------|------------|------|---------------------| | cif | No | fromInputStream | null | "" | "" | null | | pdb | No | fromInputStream | null | "" | "" | null | | cif | No | fromUrl | null | "" | "" | null | | cif | No | StructureIO | null | file: | 5PTI | file: | | pdb | No | StructureIO | null | file: | "" | file: | | cif | Yes | fromInputStream | 5PTI | "" | "" | null | | pdb | Yes | fromInputStream | 5PTI | "" | "" | null | | cif | Yes | fromUrl | 5PTI | "" | "" | null | | cif | Yes | StructureIO | 5PTI | file: | 5PTI | file: | | pdb | Yes | StructureIO | 5PTI | file: | 5PTI | file: | - `getPdbId` reflects the ID parsed from the file - Only StructureIO is setting the StructureIdentifier (by design, but indicates StructureIO should be used wherever possible) - StructureIO does not set the name properly for PDB files. This should be fixed. * Test NullPointerException when loading Alphafold models (#1019) The core URLIdentifier bug was fixed in #1020, but this improves the test. The parser changes are mostly for logging, and are there to indicate that null PdbId is an expected situation rather than an error. * Truncate file --------- Co-authored-by: Spencer Bliven <spencer.bliven@gmail.com>
3 participants
This should fix #1019 and rcsb/symmetry#113