equinor / equinor/graphite-maps
General thoughts on code
- Dominant language
- Python
- Stars
- 8
- Forks
- 3
- PR merge metrics
- No merged PRs in 30d
Description
Overall the code is quite good. Some thoughts about what might improve it:
- Refer a bit more to paper (e.g. "this line corresponds to eqn XX")
- Tighter variable naming, e.g. noisy observation is called "d", but canonical is called "canonical" and not "eta". Try to stick to mathematical names OR descriptive names, but not mix them.
- Consider removing some passthrough methods
- Consider prefixing the "low level API" methods with underscore
- Add some descriptive examples as doctests
- The verbose argument should probably be replaced with logging. Or at least reduced to just having one level and setting it once on the class.
- Perhaps split up usage into several patterns. Right now we have one set of methods that allow for many usages, which might make it hard to reason about
- Is the networkx graph needed? Might be cleaner if the public API takes into sparse matrices only, instead of the user having to juggle (1) dense matrices, (2) sparse matrices and (3) graphs. Most (all?) graph operations int he code can be expressed as linear algebra operations.
Contributor guide
Assessment
This issue has not been assessed yet.