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

🐛 QuantScores #291

Closed
enryH opened this issue Apr 25, 2024 · 4 comments
Closed

🐛 QuantScores #291

enryH opened this issue Apr 25, 2024 · 4 comments
Assignees

Comments

@enryH
Copy link
Member

enryH commented Apr 25, 2024

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

# Get quantification data
quant_score = QuantScores(
self.precursor_name, parse_settings.species_expected_ratio(), parse_settings.species_dict()
)

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

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"]
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"]
def species_dict(self):
return self.species_dict
def species_expected_ratio(self):
return self.species_expected_ratio

which is also the case for the base class:

class QuantScores:
def __init__(self, precursor_name: str, species_expected_ratio, species_dict: Dict[str, str]):
self.precursor_name = precursor_name
self.species_expected_ratio = species_expected_ratio
self.species_dict = species_dict

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

@wolski
Copy link
Contributor

wolski commented Apr 25, 2024

@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
Copy link
Member Author

enryH commented Apr 26, 2024

I ran into the error when I tried to integrate i2MassChroQ into the UI. I am not 100% sure why yet: #229

PR: #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
Copy link
Contributor

wolski commented Apr 26, 2024

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

@enryH
Copy link
Member Author

enryH commented Apr 26, 2024

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

wolski added a commit that referenced this issue Apr 26, 2024
enryH added a commit that referenced this issue Apr 26, 2024
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

No branches or pull requests

3 participants