Proteobench / ProteoBench

ProteoBench is an open and collaborative platform for community-curated benchmarks for proteomics data analysis pipelines. Our goal is to allow a continuous, easy, and controlled comparison of proteomics data analysis workflows.
https://proteobench.readthedocs.io
Apache License 2.0
27 stars 7 forks source link

:bug: QuantScores #291

Open enryH opened 2 months ago

enryH commented 2 months ago

Then I run into an error related to QuantScores:

File "C:\Users\kzl465\Documents\repos\ProteoBench\webinterface\pages\DDA_Quant_ion.py", line 238, in _run_proteobench
    result_performance, all_datapoints, input_df = self.ionmodule.benchmarking(
File "C:\Users\kzl465\Documents\repos\ProteoBench\proteobench\modules\dda_quant_ion\module.py", line 42, in benchmarking
    self.precursor_name, parse_settings.species_expected_ratio(), parse_settings.species_dict()
TypeError: 'dict' object is not callable

https://github.com/Proteobench/ProteoBench/blob/8284e9c5f8764cbf8ea0e66e40c1bb10e28eba0b/proteobench/modules/dda_quant_ion/module.py#L40-L43

Which is probably due to an attribute and the corresponding method having the same name:

https://github.com/Proteobench/ProteoBench/blob/8284e9c5f8764cbf8ea0e66e40c1bb10e28eba0b/proteobench/io/parsing/parse_settings_ion.py#L43-L63

which is also the case for the base class:

https://github.com/Proteobench/ProteoBench/blob/bc7233784a67f285f9d019c9bc08bcbc77661754/proteobench/score/quant/quantscores.py#L7-L12

So I guess removing the assessor method for the dicts should be the best solution? @wolski what was you idea to create these?

wolski commented 2 months ago

@enryH How did you run into the error? I can not replicate it. Are you on branch main?

parse_settings is not a dict but a class ParseSettings:

and we need the accessors because ParseModificationSettings and ParseSettings need to have the same interface but have different implementations:

ParseModificationSettings
def species_dict(self):
        return self.parser.species_dict

    def species_expected_ratio(self):
        return self.parser.species_expected_ratio
ParseSettings
 def species_dict(self):
        return self.species_dict

    def species_expected_ratio(self):
        return self.species_expected_ratio
enryH commented 2 months ago

I ran into the error when I tried to integrate i2MassChroQ into the UI. I am not 100% sure why yet: https://github.com/Proteobench/ProteoBench/issues/229

PR: https://github.com/Proteobench/ProteoBench/pull/279

So I am working on that branch.

I don't mind that it's implemented with methods to access the values. But you can then mask either the method or attribute depending on the execution order. Do you know who set this up intially? If we have methods, the attributes with the same name should have _ leading underscores:

 class ParseSettings: 

     """Structure that contains all the parameters used to parse 
     the given benchmark run output depending on the software tool used.""" 

     def __init__(self, parse_settings: dict[str, any], parse_settings_module: dict[str, any]): 
         self.mapper = parse_settings["mapper"] 
         self.condition_mapper = parse_settings["condition_mapper"] 
         self.run_mapper = parse_settings["run_mapper"] 
         self.decoy_flag = parse_settings["general"]["decoy_flag"] 
         self._species_dict = parse_settings["species_mapper"] `#  # changed to start with _: self._species_dict
         self.contaminant_flag = parse_settings["general"]["contaminant_flag"]  

         self.min_count_multispec = parse_settings_module["general"]["min_count_multispec"] 
         self._species_expected_ratio = parse_settings_module["species_expected_ratio"]   # changed to start with _

     def species_dict(self): 
         return self._species_dict  # changed to start with _

     def species_expected_ratio(self): 
         return self._species_expected_ratio  # changed to start with _

Are both attributes set anywhere else in the code except the constructor? (species_dict or species_expected_ratio)

wolski commented 2 months ago

AFAIK they are only set in the constructor. I will add a _ to the attributes, give me time until lunch.

enryH commented 2 months ago

Yes no hurry. I could also do it, but I think it's best to keep it a separate PR to highlight these changes.