equinor / equinor/graphite-maps

General thoughts on code

Open
#125 0 comments 0 reactions 0 assignees View on GitHub
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

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.