Skip to content

Conversation

@jonbrenas
Copy link
Collaborator

Addresses #738.

Shadows #742.

Thanks @mohamed-laarej for the code. I will remove the draft status once I checked I shadowed the PR correctly.

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@jonbrenas jonbrenas marked this pull request as ready for review April 8, 2025 15:29
@jonbrenas
Copy link
Collaborator Author

Ed and I would need this merged for the Cameroon coluzzii paper. @leehart, do you think you could find the time to review it?

@leehart leehart self-requested a review July 28, 2025 16:23
@leehart
Copy link
Collaborator

leehart commented Jul 28, 2025

@jonbrenas Why are we introducing the use of pytest-mock here? Could the tests not have been done in a way that is more consistent with the rest of the package? Adding this would mean that some tests are using mocker fixtures while others aren't. There might be a good reason, but I'd worry about having too many different test approaches in the same package.

This PR also introduces other things to the poetry lock file, e.g.

[package.dependencies]
pytest = ">=6.2.5"

[package.extras]
dev = ["pre-commit", "pytest-asyncio", "tox"]

What is the reason for those changes?

I'll try to take a closer look at the other code changes later in the week. I'm a bit suspicious about:

color="partition" if color is not None else None,

@jonbrenas
Copy link
Collaborator Author

Thanks @leehart. @mohamed-laarej will correct me if I am wrong.

Why are we introducing the use of pytest-mock here? Could the tests not have been done in a way that is more consistent with the rest of the package? Adding this would mean that some tests are using mocker fixtures while others aren't. There might be a good reason, but I'd worry about having too many different test approaches in the same package.

The haplotype networks use dash which, I think, doesn't work without some work-around.

This PR also introduces other things to the poetry lock file, e.g.

[package.dependencies]
pytest = ">=6.2.5"

[package.extras]
dev = ["pre-commit", "pytest-asyncio", "tox"]

It looks like it was added because of the inclusion of mocker but I might be wrong.

I'll try to take a closer look at the other code changes later in the week. I'm a bit suspicious about:

color="partition" if color is not None else None,

It looks a little weird but a new column "partition" is created to contain the values that the color is defined on, if there are any, so I think the code should work. It is not quite as readable as it should be, though, and it should probably be a "private" column (assuming that we have started doing that which am I am not sure we did).

@mohamed-laarej
Copy link
Collaborator

Hi @leehart and @jonbrenas ,

Thanks for reviewing this PR. My memory on the specifics is a bit rusty, but I'll do my best to address your points:

  • pytest-mock: As @jonbrenas noted, this was introduced for testing Dash components in the haplotype network function. I'm open to alternative, more consistent testing approaches if available.

  • poetry.lock changes: These updates reflect the new pytest-mock dependency and its sub-dependencies, automatically managed by Poetry for reproducible builds.

  • color="partition" line: I agree this looks a bit odd at first glance. From what I remember, the function creates a temporary "partition" column to standardize color handling regardless of how colors are specified (column name, dictionary, etc.). The line passes this standardized column name to the plotting function. You're right that it could be more readable - perhaps using a private column name like _partition would make the intent clearer.

I'm ready to make any necessary adjustments. Thanks again for the review!

Copy link
Collaborator

@leehart leehart left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @jonbrenas @mohamed-laarej . Yep, I reckon we should start using "private" column names, so let's use _partition here. If there are any other simple changes we can make to try to improve the readability here, that would be good too.

@leehart leehart self-requested a review August 5, 2025 13:33
Copy link
Collaborator

@leehart leehart left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @mohamed-laarej . Happy with this if @jonbrenas is happy.

@jonbrenas
Copy link
Collaborator Author

LGTM. Thanks @mohamed-laarej and @leehart .

@jonbrenas jonbrenas closed this Aug 6, 2025
@jonbrenas jonbrenas reopened this Aug 6, 2025
@leehart
Copy link
Collaborator

leehart commented Aug 7, 2025

@jonbrenas I suspect adding tests to tests/integration/test_ag3.py and tests/integration/test_af1.py might be contrary to issue #366 which has been removing tests from those files in favour of tests under tests/anoph, e.g. #665 which removed test_gene_cnv from tests/test_af1.py and tests/test_ag3.py. (Although those files themselves were also renamed and refactored during #686 in an attempt to separate "integration" from "unit" tests.)

In any case, I suggest that we merge this work in and potentially address migrating these tests (and other work related to #366) in a subsequent issue/PR. For instance, it may be that these tests do actually belong alongside the other tests that are currently in tests/integration/test_ag3.py and tests/integration/test_af1.py, especially because these are testing Dash components.

@leehart leehart merged commit 0532dba into master Aug 7, 2025
20 checks passed
@leehart leehart deleted the GH738-mohamed-laarej-fix-plot-haplotype-network-color branch August 7, 2025 07:59
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

Successfully merging this pull request may close these issues.

4 participants