Hi @jonbrenas ,

Thank you very much for taking the time to review this PR and for your valuable feedback!
I've pushed new commits addressing the points you raised.
Please let me know if these changes look good or if there's anything else. Thanks again!

Thanks @mohamed-laarej.

I ran a quick debug of your code and the reason why you don't get the snp_calls in phenotypes is that, in _create_phenotype_dataset, you use variant_data.coords["samples"].values as the list of sample ids from variant_data instead of variant_data.coords["sample_id"].values. I haven't looked at why you can't find the haplotypes yet but I am sure you will have it figured out before I get to it.

Thanks @mohamed-laarej. This is great!

Here are a few more comments:

• The way you have a few important functions (e.g., sample_metadata) as global variables is a little strange to me. For simplicity and clarity's sake, can you change the code so that they are inherited from the correct classes instead?
• I think the selection of the location, the insecticide, ... should be part of the sample query and not additional parameters.
• I think it would be simpler and clearer to have different functions that return the phenotypes + either the SNP calls, the haplotypes, or the CNVs (which I don't think are currently an option); or one function with a parameter that let's you select what data is included; or both, than your current approach of having that being dependent on the choice of a region. Unless you have a model in mind that will use both SNP calls and haplotypes I don't think a function that returns both is a clear improvement on one that returns either.
• A notebook showing the results of a few functions would still be very helpful.

Thanks @jonbrenas for the detailed feedback! Really helpful stuff. Let me go through each point:

Functions as Global Variables

Ah yeah, I can see why that looked weird! I ended up doing it that way because I was getting a bunch of MyPy errors when I first tried the mixin approach:

malariagen_data\anoph\phenotypes.py:31: error: "AnophelesPhenotypeData" has no attribute "_url" [attr-defined]
malariagen_data\anoph\phenotypes.py:377: error: "AnophelesPhenotypeData" has no attribute "sample_metadata" [attr-defined]

To fix this, I added type annotations and made them constructor params when you suggested adding a constructor. But after reviewing how the other mixins work, I refactored it to use cooperative inheritance like this:

class AnophelesPhenotypeData:
    # Type annotations for MyPy
    _url: str
    _fs: fsspec.AbstractFileSystem
    sample_metadata: Callable[..., pd.DataFrame]
    # etc...
    
    def __init__(self, **kwargs):
        super().__init__(**kwargs)

I think it's much cleaner! Now it just calls self.sample_metadata() directly like it should.
Could you please confirm if this is the right approach? Or is there anything else I should consider here to make it more idiomatic or robust?

Sample Query Filtering

Completely agree on this one! Consolidating everything into sample_query will definitely make the API cleaner and more consistent with how the rest of the codebase works.

Clearer Data Type Selection

This is a great point, the current logic is definitely confusing. I'd love your thoughts on which approach would work best:

Option A - Separate functions:

def phenotypes_with_snps(self, region, sample_query=None, ...):
def phenotypes_with_haplotypes(self, region, sample_query=None, analysis="arab", ...):
def phenotypes_with_cnvs(self, region, sample_query=None, ...):
def phenotypes_only(self, sample_query=None, ...):

Option B - Single function with parameter:

def phenotypes(self, region=None, genetic_data="snps", sample_query=None, ...):
    # where genetic_data can be "snps", "haplotypes", "cnvs", or None

I'm personally leaning toward Option A since it makes the intent really clear and avoids any ambiguity, but I'd definitely defer to your judgment on what would be most intuitive for users.

Next Steps

I'll get started on:
Cleaning up the inheritance structure based on your guidance
Moving all filtering logic to sample_query
Implementing whichever genetic data selection approach you prefer
Putting together a demo notebook to show how everything works

Thanks again for taking the time to provide such detailed feedback, it's really helpful for understanding the codebase patterns better!

Thanks @mohamed-laarej. I think you got that right. Option A would probably be my choice: if we want to add more options later on, it will be clearer to add a new function than an accepted value for a parameter.

Check out this pull request on 

See visual diffs & provide feedback on Jupyter Notebooks.

Powered by ReviewNB

Hi @jonbrenas ,
Thanks again for your valuable feedback!

I've addressed the points regarding:

1. Making location, insecticide, etc., part of the sample_query parameter.
2. Splitting the phenotypes function into phenotypes_with_snps, phenotypes_with_haplotypes, and a dedicated phenotype_data function for phenotype-only retrieval.
3. A notebook (phenotype_data_demo.ipynb) to show how to use the new functionalities.

Regarding the CNVs, I did attempt to integrate them using cnv_coverage_calls. However, I ran into some persistent issues, mainly related to the specific cnv_analysis availability for certain sample sets and the cnv_coverage_calls API expecting a single sample_set rather than a list. I haven't quite figured out how to make it work for now, but I will continue to try and see if I can get it integrated.

I've always assumed that the blank line between imports at the top of ag3.py is used to visually separate external from internal imports. For comparison, there are also blank lines between the imports in af1.py, which I took to be visual separations for standard library modules (e.g. sys) and external modules (e.g. plotly.express) and internal modules (e.g. .anopheles).

This might be a style choice and does not need to be changed in this PR. Although the style does not seem to be applied consistently everywhere (e.g. anoph/aim_data.py), perhaps because it was never written as policy.

If we decide to police "don't have lines between imports", then that would require a lot of changes. Normally this wouldn't even be noticeable or noteworthy, but there is no other change in ag3.py in this PR.

Thanks @leehart for the observation! To be honest, I hadn't noticed the specific pattern you're referring to in the import sections before. I tend to add blank lines wherever I feel they improve readability or visually separate blocks of code, not following any strict convention. You're right about the pattern in af1.py; I can see how those blank lines help distinguish between standard library, external, and internal imports.
That said, I appreciate you bringing this up a consistent approach across the codebase does make sense if the team ever decides to formalize it. For now, I've left things as-is since this PR is focused on new functionality, and standardizing import styling seems like something that might be better addressed team-wide.

Hi @jonbrenas, I just wanted to give you a quick heads-up
While reviewing the PR after approval (thank you again!), I realized that the phenotype_binary method had been inadvertently removed during the recent refactoring around sample_query and phenotype_data, and I unfortunately overlooked re-adding it.
Since this method was part of the original functionality and provides a convenient way to access binary phenotype outcomes, I’ve restored it in line with the new design principles, it now uses sample_query for filtering. I’ve also added integration tests and updated the demo notebook with relevant examples.
Apologies for the late addition to an already-approved PR. If you’d prefer this be handled in a separate PR instead, I’m happy to make that change just let me know.
Thanks again for your time and understanding!

That's good thanks @mohamed-laarej !