Skip to content

Fix name collision in maximum_clique#146

Open
timholy wants to merge 1 commit into
mojaie:masterfrom
timholy:teh/collision
Open

Fix name collision in maximum_clique#146
timholy wants to merge 1 commit into
mojaie:masterfrom
timholy:teh/collision

Conversation

@timholy
Copy link
Copy Markdown
Contributor

@timholy timholy commented Mar 19, 2026

Graphs v1.14.0 started exporting maximum_clique, which is a function that MolecularGraph also exports. The two names collide, which causes problems for MolecularGraph's test suite.

This "fixes" the problem by merging the two functions, having MolecularGraph add new methods to the Graphs function. Currently, this PR should be viewed as a "trial balloon" because there is a major problem: solving the problem this way is piracy. There are several alternatives, including (1) renaming MolecularGraph's function to something else, (2) scoping calls to MolecularGraph.maximum_clique in the test suite, or (3) defining a new function in MolecularGraph that calls Graphs' function and then does some post-processing. Let me know your preferred solution and I can implement it.

Because this was introducing piracy, I also took the liberty of adding Aqua tests. Currently this highlights quite a few failures (not just the piracy I introduced), so I marked the test as broken. Fixing these should be the subject of a future PR.

@timholy
Copy link
Copy Markdown
Contributor Author

timholy commented Mar 31, 2026

This ended up being trickier than expected because this package still supports Julia 1.9, but Graphs no longer does, so maximum_clique may or may not be defined by Graphs depending on the Julia version. But this version finally passes tests.

Again, I'm happy to make revisions when you let me know how you'd like to handle the piracy issue.

@mojaie
Copy link
Copy Markdown
Owner

mojaie commented Apr 12, 2026

I'm sorry for the late response.
The minimum version will be bumped up to the latest LTS release v1.10.11 (March 9, 2026).

This package has some common graph algorithms that should be implemented in Graphs.jl as menthoned on #128. But there should be some specific algorithms for molecular mining tasks. In the case of maximum_clique, Graphs.jl implementation seems not support lazy enumeration of cliques and timeouts that are essential for practical use.

My simple idea is to rename conflicting functions in this package (as you mentioned as (1)).
Looking ahead, I think it would be a good idea to establish a naming convention for functions with the same name as those in Graphs.jl that clearly indicates they have been intentionally implemented to handle molecular mining tasks (e.g. maximum_clique_mg).

By just adding aliases as like

maximum_clique_mg = MolecularGraph.maximum_clique

Graphs v1.14.0 started exporting `maximum_clique`, which is a function
that MolecularGraph also exports. The two names collide, which causes
problems for MolecularGraph's test suite.

This fixes the problem by renaming MolecularGraph's `maximum_clique` to
`maximum_clique_mg`.

This also adds a bunch of Aqua tests to check for piracy, etc.
@timholy
Copy link
Copy Markdown
Contributor Author

timholy commented May 18, 2026

Sorry for the delay in getting back to this. I've adopted the maximum_clique_mg naming and updated the PR.

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.

2 participants