search

Chemellia / ChemistryFeaturization.jl

Interface package for featurizing atomic structures
https://chemistryfeaturization.chemellia.org/dev/
MIT License
41 stars 14 forks source link

Use f32 for type stability #35

Closed DhairyaLGandhi closed 3 years ago

DhairyaLGandhi commented 3 years ago

This doesn't mean much since we're not differentiating through this function (correct me if I'm wrong), but results in slightly clearer code, with type stability for the Float32 case which seems to be the default.

if we we're differentiating through this, we would also have seen a speedup in the backwards pass (g isa AtomGraph with a 7x10 graph) 

julia> @btime gradient(gr -> sum(normalized_laplacian(gr)), $(g.graph));
  82.412 μs (433 allocations: 17.92 KiB)

julia> @btime gradient(gr -> sum(normalized_laplacian2(gr)), $(g.graph));
  33.066 μs (108 allocations: 8.41 KiB)
DhairyaLGandhi commented 3 years ago

The failure seems to be local to Windows and seems unrelated to the PR

rkurchin commented 3 years ago

Has something changed about the windows-latest test setup then? Because nothing in the code has changed since the tests passed last...we should probably still sort out what's causing it before merging this, no?

DhairyaLGandhi commented 3 years ago

That's an orthogonal issue, and unrelated, so I wouldn't block the PR over that. I haven't touched anything in the tests either, and it seems like it is missing a file (wrong path? Corrupted download?) I'll rerun ci.

rkurchin commented 3 years ago

Crap, now rdkit is complaining again...I thought pinning the version fixed that problem...these issues are so annoying to troubleshoot because I have a lot of trouble reproducing them locally...

rkurchin commented 3 years ago

I'm going to just merge this and troubleshoot the testing issues on the other PR