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

plot_frequencies_heatmap is not in the correct file #664

Open
jonbrenas opened this issue Nov 25, 2024 · 3 comments
Open

plot_frequencies_heatmap is not in the correct file #664

jonbrenas opened this issue Nov 25, 2024 · 3 comments
Assignees
Labels
maintenance question Further information is requested

Comments

@jonbrenas
Copy link
Collaborator

plot_frequencies_heatmap is in snp_frq.py when it is used to plot all kinds of frequencies heat maps. It could probably be moved to util.py (if there are parasite functions that want to use a similar function) or anopheles.py.

This issue replaces the second part of #661.

Here is @leehart comment on this issue from #661:

I'm less comfortable with migrating plot_frequencies_heatmap to util.py or anopheles.py. I'm wary about files like util.py becoming a general purpose catch-all place for all kinds of stuff, like that stereotypical one kitchen drawer! And if we're talking about moving stuff out of anopheles.py, which sounds like a pretty high-level place, something seems wrong about moving other stuff into it, especially plotting functions, within the same breath. But I would need to refresh my memory on the scope of those particular modules. In principle, I would go with grouping like-with-like, but you know those sort of decisions can become subjective or aesthetic, rather than perfectly rational.

Where do we usually put functions (e.g. plotting functions) that are used by both parasite and vector? Is it really util.py?

@jonbrenas jonbrenas self-assigned this Nov 25, 2024
@jonbrenas jonbrenas added question Further information is requested maintenance labels Nov 25, 2024
@jonbrenas
Copy link
Collaborator Author

jonbrenas commented Nov 25, 2024

I see what you mean @leehart and I agree. I don't really think there are many (if any) functions that are used by both parasite and vector. I think a good alternative would be to create a new file, e.g. frq_functions.py, that contains the functions that are shared between hap_frq.py, snp_frq.py and (future) cnv_frq.py. Would that make sense @leehart, @alimanfoo ?

@jonbrenas
Copy link
Collaborator Author

jonbrenas commented Nov 25, 2024

Worth mentioning, #630 (the PR that creates hap_freq.py) moves a lot of functions (but not the plotting functions) that are shared by all 3 _advanced versions of the frequency analyses to util.py. I think that it would make more sense to move them to frq_functions.py as well if we choose to go this way.

@jonbrenas
Copy link
Collaborator Author

I made most of the changes that I wanted ... and realised that they might not be what I wanted.

I think that, instead of a separate test_frq.py containing its tests on general functions, it would be better to have general functions that are used in the various test_*_frq.pys. @alimanfoo says that pytest is a little peculiar about importing test functions so it might not be as simple as I hoped, though. This probably needs more discussion so I'll leave it be for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintenance question Further information is requested
Projects
None yet
Development

No branches or pull requests

1 participant