search

mojaie / MolecularGraph.jl

Graph-based molecule modeling toolkit for cheminformatics
MIT License
189 stars 27 forks source link

precalculate! doesn't cache isaromatic(mol::MolGraph) property. #86

Closed Boxylmer closed 1 year ago

Boxylmer commented 1 year ago

Hi Mojaie! I'm sure at this point you're sick of me opening issues, haha.

I was doing performance benchmarks with our project and noticed isaromatic() taking up a whopping 1/3 of our processing time. After a bit of investigation, I found that it wasn't cached during precalculate! calls. I read that this function is supposed to only cache bottlenecking behavior. So I wanted to ask if this was the intended functionality before I try to fix it. Should it be caching the aromaticity of atoms?

https://github.com/mojaie/MolecularGraph.jl/blob/3a687aa77350733870945ce75fe9fedae18f7bb8/src/properties.jl#L707-L715

mojaie commented 1 year ago

I'm very sorry for the late response. As you guessed, precalculate! caches bottleneck functions in descriptor calculation.

https://github.com/mojaie/MolecularGraph.jl/issues/22

Cache mechanism may be very different in the next version (working on new_graph branch), and I planed to do benchmarking for that. Please wait for a while until the new version.

Boxylmer commented 1 year ago

Haha, no need to apologize! And I'll plan for those changes and keep up with the branch! I just cached manually in the meantime, it wasn't a difficult fix at all. Looking forward to the updates!