Open
Conversation
The `_parse_scalar` was never called for the vector arguments. The parse vectorlist iteration was made simpler
…om classmethod Since the ET cant be parsed if the file doesnt exist, not clear how the check would fail
if it were a classmethod, it should return a new instance of a class, but this returns a `SimpleNamespace`
Contributor
seongsujeong
left a comment
There was a problem hiding this comment.
Thanks for the PR @scottstanie. I think the updates here are important to avoid the race condition when concurrent threads exists like the example you posted here.
Here are my comments and suggestions - Seongsu
src/s1reader/s1_annotation.py
Outdated
Comment on lines
453
to
476
| 'datetime') | ||
| cls.number_of_samples = \ | ||
| cls._parse_scalar('imageAnnotation/imageInformation/numberOfSamples', | ||
| number_of_samples = \ | ||
| cls._parse_scalar(et_in, | ||
| 'imageAnnotation/imageInformation/numberOfSamples', | ||
| 'scalar_int') | ||
| cls.number_of_samples = \ | ||
| cls._parse_scalar('imageAnnotation/imageInformation/numberOfSamples', | ||
| number_of_samples = \ | ||
| cls._parse_scalar(et_in, | ||
| 'imageAnnotation/imageInformation/numberOfSamples', | ||
| 'scalar_int') |
Contributor
There was a problem hiding this comment.
Looks like it's my bad again. It seems like that we have a duplicate here.
scottstanie
commented
Jun 29, 2023
Co-authored-by: Seongsu Jeong <sjeong.kr@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This is trying to prevent #122 from popping up sporadically.
This is changing the
classmethods in the annotation module to return instances: https://stackoverflow.com/a/14605349/4174466All functions that are decorated with
@classmethodare supposed to be alternate constructors (in addition to__init__); that is, they're meant to create a new instance. Right now, they have been changing "class attributes", which are shared across all instances of that class, and they are returning the class itself.Why this is a problem
If we kick of many instance of COMPASS in different threads, each one that reads bursts instantiates these annotation classes. they are all changing the same class attribute, so it's a data race where difference function calls to
.from_etcan clobber each other.(example to show what this looks like)
Details
if you try to do this method in many threads: