Global norm and map#838
Conversation
|
@John-F-Wagstaff , see above for failing tests in expanded_repeats.py, othewise this is passing all tests and knocked about 23 seconds off testing |
John-F-Wagstaff
left a comment
John-F-Wagstaff
left a comment
There was a problem hiding this comment.
Looks good so far, but needs the final fixes for expanded repeats
@Peter-J-Freeman do you want me to look at getting the expanded repeats plugged in myself and commit onto this branch? or just dig up the issue and pass you the howto?
|
If you could plug it in while making your edits to global variables please. It looks to me like it might be related to where things end up being placed, so maybe better to pull this branch and continue the restructure on it. |
|
|
||
|
|
||
| # Create additional normalizers | ||
| def create_additional_normalizers_and_mappers(self): |
There was a problem hiding this comment.
So I skipped past this code on my first read through, but having had more time to think we need to replace this.
We can't have a self.reverse_hn, self.hn, self.merge_normalizer, or self.reverse_merge_normalizer set globally on the Validator object, from a self.alt_aln_method because self.alt_aln_method should not exist as a global Validator object variable. We either need to set them on the Variant object (as pointers to the already created versions exactly as we do here) or use them based on the state of self.alt_aln_method at the usage points (leaving me to clean that up later, or keep them as local variables for the main validation function in vvMixinCore and pass them around that way.
I could do this as part of settings de-globalisation followup to this though if we need to.
There was a problem hiding this comment.
If you want to leave in the Variant object specific ones above ( in VariantValidator/modules/variant.py ) then just removing all of these in favour of those might be the best method.
There was a problem hiding this comment.
Makes sense, so move them into the variant object. I did wonder. This is why I started to think that it makes sense to add it to the global vs local code restructuring.
The framework is in place at least. Yes, I think it makes sense now for you to take over and move things in terms of global, vs local, vs validator object vs variant object.
There was a problem hiding this comment.
If you have decided that you want to leave this code here then that is fine, but if it is OK I would rather add a final patch to this branch, to get it all working as is, and pull that, rather than merge this in to develop with broken expanded repeats.
|
|
||
| # Create additional normalizers | ||
| def create_additional_normalizers_and_mappers(self): | ||
| def get_normalizers(self): |
There was a problem hiding this comment.
These probably all need changing as with the Mappers, they either need to be added to the Variant, made local to the main validation function or removed in favor or checking self.alt_aln_method at the usage point (for me to fix the self.alt_aln_method usage later)
There was a problem hiding this comment.
Yes, I think again it makes sense to let you take over now. The framework is in place. Its all about restructuring now
Depending on how you want to resolve the other comments you could just put in a call to set the mappers on the Variant before calling this code, and then use the mappers from the Variant instead of creating them in this file. |
Peter-J-Freeman
force-pushed
the
global_norm_and_map
branch
from
5786286 to
20d1644
Compare
This change configures the normalizer objects in the core validator object. The reason for the change is optimised resource management and performance (by removing normalizer setup from the validation loop).
Mapping objects are now setup in the validator objecta offering a performance update (removing mapper setup from the validation loop). This also makes resource management more predictable.
Peter-J-Freeman
force-pushed
the
global_norm_and_map
branch
from
20d1644 to
05c0051
Compare
|
@John-F-Wagstaff. Commit messages fixed up This is where I was confused. The validator objects are set up early in the variant object. I could not figure out how |
| for my_variant in self.batch_list: | ||
|
|
||
| # Create Normalizers | ||
| my_variant.hn = vvhgvs.normalizer.Normalizer(self.hdp, |
There was a problem hiding this comment.
If you want the mappers in place during the original parsing of the input (which is needed for expanded repeats) then they need to be set here. This is also where the set appropriate normaliser code for the Variant object needs to be triggered (instead of the new normaliser creation that we have below this line).
2 participants
PR merges in the use of global normalizers and mappers
@John-F-Wagstaff, tests for exon boundary crossing in expanded_repeats.py
are failing. It is not clear to me how to pass the global mappers correctly.