Chemellia / ChemistryFeaturization.jl

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

reorganization, make into interfaces... #132

Closed rkurchin closed 2 years ago

rkurchin commented 2 years ago

This is currently a WIP where I'm implementing a bunch of the stuff we've discussed vis a vis splitting out functionalities, "interface-ifying" things, and further separating concerns wrt the codec syntax. Some broad remarks..

OIther specific changes:

Interfaces and associated functions:

Remaining todos:

rkurchin commented 2 years ago

NB's for people:

The only concrete FD I'm keeping in CF for now is the EFD since that's quite generic and also depends very little on the nature of the structure representation. But I may split out modules for different sources of lookup tables e.g. pymatgen, matminer, etc...

thazhemadam commented 2 years ago

@rkurchin test failures seem to be primarily because we are still using Conda in deps/build.jl, when Conda.jl was removed as a dependency in 352044ffd4ad2829c82bfc348851c56c38ded5c8.

rkurchin commented 2 years ago

Thanks, yeah I haven't finished fixing all the tests yet. Will keep that in mind, hoping to get to it later today!

codecov[bot] commented 2 years ago

Codecov Report

Merging #132 (899fbac) into main (ef30f5f) will increase coverage by 4.38%. The diff coverage is 86.72%.

:exclamation: Current head 899fbac differs from pull request most recent head 08cca74. Consider uploading reports for the commit 08cca74 to get more accurate results

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #132      +/-   ##
==========================================
+ Coverage   80.80%   85.18%   +4.38%     
==========================================
  Files          20       11       -9     
  Lines         573      162     -411     
==========================================
- Hits          463      138     -325     
+ Misses        110       24      -86     
Impacted Files Coverage Δ
src/ChemistryFeaturization.jl 0.00% <0.00%> (-80.00%) :arrow_down:
src/featurizedatoms.jl 44.44% <66.66%> (+4.44%) :arrow_up:
src/codecs/onehotonecold_utils.jl 78.94% <78.94%> (ø)
src/codecs/onehotonecold.jl 84.44% <83.33%> (+0.23%) :arrow_up:
src/features/features.jl 80.00% <87.50%> (-8.89%) :arrow_down:
src/codecs/codecs.jl 100.00% <100.00%> (ø)
src/codecs/directcodec.jl 100.00% <100.00%> (ø)
src/codecs/simplecodec.jl 100.00% <100.00%> (ø)
src/features/elementfeature.jl 100.00% <100.00%> (+15.00%) :arrow_up:
src/features/elementfeature_utils.jl 100.00% <100.00%> (ø)
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update ef30f5f...08cca74. Read the comment docs.

rkurchin commented 2 years ago

I'd like to go ahead and get this merged so that I can register and then merge/register the associated AtomGraphs and AGN updates (and @DhairyaLGandhi can do so with WeaveModel as well). Once everything is registered I'll be able to fix the docs issues so that they can build again.

Anyway, would love any additional feedback you guys have here! I think we were generally in agreement about these changes. so I may just merge in a day or so if I don't hear any objections.

rkurchin commented 2 years ago

I'm generally on board with the utils/ directory idea, but it doesn't seem necessary to me when there would only be one file in it. I think if/when we add more such things we can introduce that level to the hierarchy.